Change in coreboot[master]: soc/intel/common/fast_spi: Add support for configuring MTRRs
Srinidhi N Kaushik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47991 )
Change subject: soc/intel/common/fast_spi: Add support for configuring MTRRs ......................................................................
soc/intel/common/fast_spi: Add support for configuring MTRRs
This change enables caching for extended BIOS region. Currently, caching is enabled for the standard BIOS region upto a maximum of 16MiB using fast_spi_cache_bios_region, used the same function to add the support for caching for extended BIOS region as well.
Changes include: 1. Add a new helper function fast_spi_cache_ext_bios_region() which calls fast_spi_get_ext_bios_window() to get details about the extended BIOS window from the boot media map. 2. Make a call to fast_spi_cache_ext_bios_region() from fast_spi_cache_bios_region (). 3. If the extended window is used, then it enables caching for this window similar to how it is done for the standard window.
BUG=b:171534504
Signed-off-by: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Change-Id: I9711f110a35a167efe3a4c912cf46c63c0812779 --- M src/soc/intel/common/block/fast_spi/fast_spi.c 1 file changed, 37 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/47991/1
diff --git a/src/soc/intel/common/block/fast_spi/fast_spi.c b/src/soc/intel/common/block/fast_spi/fast_spi.c index a3ec0d7..d5c6218 100644 --- a/src/soc/intel/common/block/fast_spi/fast_spi.c +++ b/src/soc/intel/common/block/fast_spi/fast_spi.c @@ -213,6 +213,40 @@ return bios_start; }
+static void fast_spi_cache_ext_bios_window(void) +{ + + size_t ext_bios_size; + uint32_t alignment; + uintptr_t ext_bios_base; + const int type = MTRR_TYPE_WRPROT; + + if (!CONFIG(FAST_SPI_SUPPORTS_EXT_BIOS_WINDOW)) + return; + + fast_spi_get_ext_bios_window(&ext_bios_base, &ext_bios_size); + + /* Enable extended bios only if Size of Bios region is greater than 16MiB */ + if (ext_bios_size == 0 || ext_bios_base == 0) + return; + + /* Round to power of two */ + alignment = 1UL << (log2_ceil(ext_bios_size)); + ext_bios_size = ALIGN_UP(ext_bios_size, alignment); + ext_bios_base = ALIGN_DOWN(ext_bios_base, ext_bios_size); + + if (ENV_PAYLOAD_LOADER) { + mtrr_use_temp_range(ext_bios_base, ext_bios_size, type); + } else { + int mtrr = get_free_var_mtrr(); + + if (mtrr == -1) + return; + + set_var_mtrr(mtrr, ext_bios_base, ext_bios_size, type); + } +} + void fast_spi_cache_bios_region(void) { size_t bios_size; @@ -246,6 +280,9 @@
set_var_mtrr(mtrr, base, bios_size, type); } + + /* Check if caching is needed for extended bios region if supported */ + fast_spi_cache_ext_bios_window(); }
/* Enable extended bios support
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47991 )
Change subject: soc/intel/common/fast_spi: Add support for configuring MTRRs ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/47991/1/src/soc/intel/common/block/... File src/soc/intel/common/block/fast_spi/fast_spi.c:
https://review.coreboot.org/c/coreboot/+/47991/1/src/soc/intel/common/block/... PS1, Line 238: if (ENV_PAYLOAD_LOADER) { : mtrr_use_temp_range(ext_bios_base, ext_bios_size, type); : } else { : int mtrr = get_free_var_mtrr(); : : if (mtrr == -1) : return; : : set_var_mtrr(mtrr, ext_bios_base, ext_bios_size, type); : } nit: This can be converted into a helper function `fast_spi_configure_mtrr()` and used here and in fast_spi_cache_bios_region.
Srinidhi N Kaushik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47991 )
Change subject: soc/intel/common/fast_spi: Add support for configuring MTRRs ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47991/1/src/soc/intel/common/block/... File src/soc/intel/common/block/fast_spi/fast_spi.c:
https://review.coreboot.org/c/coreboot/+/47991/1/src/soc/intel/common/block/... PS1, Line 238: if (ENV_PAYLOAD_LOADER) { : mtrr_use_temp_range(ext_bios_base, ext_bios_size, type); : } else { : int mtrr = get_free_var_mtrr(); : : if (mtrr == -1) : return; : : set_var_mtrr(mtrr, ext_bios_base, ext_bios_size, type); : }
nit: This can be converted into a helper function `fast_spi_configure_mtrr()` and used here and in f […]
Ack
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47991 )
Change subject: soc/intel/common/fast_spi: Add support for configuring MTRRs ......................................................................
Patch Set 1: -Code-Review
(1 comment)
https://review.coreboot.org/c/coreboot/+/47991/1/src/soc/intel/common/block/... File src/soc/intel/common/block/fast_spi/fast_spi.c:
https://review.coreboot.org/c/coreboot/+/47991/1/src/soc/intel/common/block/... PS1, Line 238: if (ENV_PAYLOAD_LOADER) { : mtrr_use_temp_range(ext_bios_base, ext_bios_size, type); : } else { : int mtrr = get_free_var_mtrr(); : : if (mtrr == -1) : return; : : set_var_mtrr(mtrr, ext_bios_base, ext_bios_size, type); : }
Ack
Actually, let's not add a helper function right now. For extended BIOS window, we don't need the check:
if (ENV_PAYLOAD_LOADER) { ... }
Instead we need to add a separate helper `fast_spi_cache_ext_bios_postcar()` which adds the MTRR to postcar frame. I can push that as a follow-up.
For this change, I would say let's drop the if-part of this function. I think we can do the same for `fast_spi_cache_bios_region()` too, but it is currently being used by APL. I will have to read the code to ensure that it is just a leftover and then drop it. I can clean that up as a follow-up.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47991 )
Change subject: soc/intel/common/fast_spi: Add support for configuring MTRRs ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47991/1/src/soc/intel/common/block/... File src/soc/intel/common/block/fast_spi/fast_spi.c:
https://review.coreboot.org/c/coreboot/+/47991/1/src/soc/intel/common/block/... PS1, Line 238: if (ENV_PAYLOAD_LOADER) { : mtrr_use_temp_range(ext_bios_base, ext_bios_size, type); : } else { : int mtrr = get_free_var_mtrr(); : : if (mtrr == -1) : return; : : set_var_mtrr(mtrr, ext_bios_base, ext_bios_size, type); : }
Actually, let's not add a helper function right now. […]
BTW, if you want to add the helper fast_spi_cache_ext_bios_postcar as part of this function, please feel free to do that. You will have to refactor this function such that:
static bool fast_spi_ext_bios_cache_range(uintptr_t *base, size_t size) {
if (!CONFIG(FAST_SPI_SUPPORTS_EXT_BIOS_WINDOW)) return false;
fast_spi_get_ext_bios_window(base, size);
/* Enable extended bios only if Size of Bios region is greater than 16MiB */ if (*base == 0 || *size == 0) return false;
/* Round to power of two */ alignment = 1UL << (log2_ceil(*size)); *size = ALIGN_UP(*size, alignment); *base = ALIGN_DOWN(*base, *size);
return true; }
and then use that in both fast_spi_cache_ext_bios_window and fast_spi_cache_ext_bios_postcar.
fast_spi_cache_ext_bios_window will use it for set_var_mtrr and fast_spi_cache_ext_bios_postcar will use it for postcar_frame_add_mtrr
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47991
to look at the new patch set (#2).
Change subject: soc/intel/common/fast_spi: Add support for configuring MTRRs ......................................................................
soc/intel/common/fast_spi: Add support for configuring MTRRs
This change enables caching for extended BIOS region. Currently, caching is enabled for the standard BIOS region upto a maximum of 16MiB using fast_spi_cache_bios_region, used the same function to add the support for caching for extended BIOS region as well.
Changes include: 1. Add a new helper function fast_spi_cache_ext_bios_region() which calls fast_spi_get_ext_bios_window() to get details about the extended BIOS window from the boot media map. 2. Make a call to fast_spi_cache_ext_bios_region() from fast_spi_cache_bios_region (). 3. If the extended window is used, then it enables caching for this window similar to how it is done for the standard window.
BUG=b:171534504
Signed-off-by: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Change-Id: I9711f110a35a167efe3a4c912cf46c63c0812779 --- M src/soc/intel/common/block/fast_spi/fast_spi.c 1 file changed, 38 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/47991/2
Srinidhi N Kaushik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47991 )
Change subject: soc/intel/common/fast_spi: Add support for configuring MTRRs ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47991/1/src/soc/intel/common/block/... File src/soc/intel/common/block/fast_spi/fast_spi.c:
https://review.coreboot.org/c/coreboot/+/47991/1/src/soc/intel/common/block/... PS1, Line 238: if (ENV_PAYLOAD_LOADER) { : mtrr_use_temp_range(ext_bios_base, ext_bios_size, type); : } else { : int mtrr = get_free_var_mtrr(); : : if (mtrr == -1) : return; : : set_var_mtrr(mtrr, ext_bios_base, ext_bios_size, type); : }
BTW, if you want to add the helper fast_spi_cache_ext_bios_postcar as part of this function, please […]
I have refactored and added fast_spi_cache_ext_bios_window, for postcar I was not sure how to populate pcf. Should I call fill_postcar_frame ?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47991 )
Change subject: soc/intel/common/fast_spi: Add support for configuring MTRRs ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47991/1/src/soc/intel/common/block/... File src/soc/intel/common/block/fast_spi/fast_spi.c:
https://review.coreboot.org/c/coreboot/+/47991/1/src/soc/intel/common/block/... PS1, Line 238: if (ENV_PAYLOAD_LOADER) { : mtrr_use_temp_range(ext_bios_base, ext_bios_size, type); : } else { : int mtrr = get_free_var_mtrr(); : : if (mtrr == -1) : return; : : set_var_mtrr(mtrr, ext_bios_base, ext_bios_size, type); : }
I have refactored and added fast_spi_cache_ext_bios_window, for postcar I was not sure how to popula […]
Yes, the flow will have to be:
fill_postcar_frame() --> fast_spi_cache_ext_bios_postcar --> fast_spi_ext_bios_cache_range --> postcar_frame_add_mtrr
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47991
to look at the new patch set (#3).
Change subject: soc/intel/common/fast_spi: Add support for configuring MTRRs ......................................................................
soc/intel/common/fast_spi: Add support for configuring MTRRs
This change enables caching for extended BIOS region. Currently, caching is enabled for the standard BIOS region upto a maximum of 16MiB using fast_spi_cache_bios_region, used the same function to add the support for caching for extended BIOS region as well.
Changes include: 1. Add a new helper function fast_spi_cache_ext_bios_window() which calls fast_spi_ext_bios_cache_range() which calls fast_spi_get_ext_bios_window() to get details about the extended BIOS window from the boot media map and checks for allignment and set mtrr. 2. Make a call to fast_spi_cache_ext_bios_region() from fast_spi_cache_bios_region (). 3. Add new helper function fast_spi_cache_ext_bios_postcar() which does caching ext BIOS region in postcar similar to 1. 4. If the extended window is used, then it enables caching for this window similar to how it is done for the standard window.
BUG=b:171534504
Signed-off-by: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Change-Id: I9711f110a35a167efe3a4c912cf46c63c0812779 --- M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/include/intelblocks/fast_spi.h M src/soc/intel/common/block/systemagent/memmap.c 3 files changed, 60 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/47991/3
Srinidhi N Kaushik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47991 )
Change subject: soc/intel/common/fast_spi: Add support for configuring MTRRs ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47991/1/src/soc/intel/common/block/... File src/soc/intel/common/block/fast_spi/fast_spi.c:
https://review.coreboot.org/c/coreboot/+/47991/1/src/soc/intel/common/block/... PS1, Line 238: if (ENV_PAYLOAD_LOADER) { : mtrr_use_temp_range(ext_bios_base, ext_bios_size, type); : } else { : int mtrr = get_free_var_mtrr(); : : if (mtrr == -1) : return; : : set_var_mtrr(mtrr, ext_bios_base, ext_bios_size, type); : }
Yes, the flow will have to be: […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47991 )
Change subject: soc/intel/common/fast_spi: Add support for configuring MTRRs ......................................................................
Patch Set 4: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/47991/4/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/fast_spi.h:
https://review.coreboot.org/c/coreboot/+/47991/4/src/soc/intel/common/block/... PS4, Line 86: /* : * Postcar cache for extended BIOS region : */ Single line comment: /* ... */
Also probably say: Add MTRR for extended BIOS region (if required) to postcar frame.
https://review.coreboot.org/c/coreboot/+/47991/4/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/memmap.c:
https://review.coreboot.org/c/coreboot/+/47991/4/src/soc/intel/common/block/... PS4, Line 76: thee nit: the
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47991
to look at the new patch set (#5).
Change subject: soc/intel/common/fast_spi: Add support for configuring MTRRs ......................................................................
soc/intel/common/fast_spi: Add support for configuring MTRRs
This change enables caching for extended BIOS region. Currently, caching is enabled for the standard BIOS region upto a maximum of 16MiB using fast_spi_cache_bios_region, used the same function to add the support for caching for extended BIOS region as well.
Changes include: 1. Add a new helper function fast_spi_cache_ext_bios_window() which calls fast_spi_ext_bios_cache_range() which calls fast_spi_get_ext_bios_window() to get details about the extended BIOS window from the boot media map and checks for allignment and set mtrr. 2. Make a call to fast_spi_cache_ext_bios_region() from fast_spi_cache_bios_region (). 3. Add new helper function fast_spi_cache_ext_bios_postcar() which does caching ext BIOS region in postcar similar to 1. 4. If the extended window is used, then it enables caching for this window similar to how it is done for the standard window.
BUG=b:171534504
Signed-off-by: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Change-Id: I9711f110a35a167efe3a4c912cf46c63c0812779 --- M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/include/intelblocks/fast_spi.h M src/soc/intel/common/block/systemagent/memmap.c 3 files changed, 60 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/47991/5
Srinidhi N Kaushik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47991 )
Change subject: soc/intel/common/fast_spi: Add support for configuring MTRRs ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47991/4/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/fast_spi.h:
https://review.coreboot.org/c/coreboot/+/47991/4/src/soc/intel/common/block/... PS4, Line 86: /* : * Postcar cache for extended BIOS region : */
Single line comment: /* ... */ […]
I tried to keep the comment consistent with the other single line comments in this file, I can change it to single line, let me know.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47991 )
Change subject: soc/intel/common/fast_spi: Add support for configuring MTRRs ......................................................................
Patch Set 7: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/47991/4/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/fast_spi.h:
https://review.coreboot.org/c/coreboot/+/47991/4/src/soc/intel/common/block/... PS4, Line 86: /* : * Postcar cache for extended BIOS region : */
I tried to keep the comment consistent with the other single line comments in this file, I can chang […]
Ack.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47991 )
Change subject: soc/intel/common/fast_spi: Add support for configuring MTRRs ......................................................................
Patch Set 8: Code-Review+2
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47991 )
Change subject: soc/intel/common/fast_spi: Add support for configuring MTRRs ......................................................................
soc/intel/common/fast_spi: Add support for configuring MTRRs
This change enables caching for extended BIOS region. Currently, caching is enabled for the standard BIOS region upto a maximum of 16MiB using fast_spi_cache_bios_region, used the same function to add the support for caching for extended BIOS region as well.
Changes include: 1. Add a new helper function fast_spi_cache_ext_bios_window() which calls fast_spi_ext_bios_cache_range() which calls fast_spi_get_ext_bios_window() to get details about the extended BIOS window from the boot media map and checks for allignment and set mtrr. 2. Make a call to fast_spi_cache_ext_bios_region() from fast_spi_cache_bios_region (). 3. Add new helper function fast_spi_cache_ext_bios_postcar() which does caching ext BIOS region in postcar similar to 1. 4. If the extended window is used, then it enables caching for this window similar to how it is done for the standard window.
BUG=b:171534504
Signed-off-by: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Change-Id: I9711f110a35a167efe3a4c912cf46c63c0812779 Reviewed-on: https://review.coreboot.org/c/coreboot/+/47991 Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/include/intelblocks/fast_spi.h M src/soc/intel/common/block/systemagent/memmap.c 3 files changed, 60 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/soc/intel/common/block/fast_spi/fast_spi.c b/src/soc/intel/common/block/fast_spi/fast_spi.c index 24c667b..d6e8f53 100644 --- a/src/soc/intel/common/block/fast_spi/fast_spi.c +++ b/src/soc/intel/common/block/fast_spi/fast_spi.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+#include <arch/romstage.h> #include <device/mmio.h> #include <assert.h> #include <device/pci_def.h> @@ -212,6 +213,53 @@ return bios_start; }
+static bool fast_spi_ext_bios_cache_range(uintptr_t *base, size_t *size) +{ + uint32_t alignment; + if (!CONFIG(FAST_SPI_SUPPORTS_EXT_BIOS_WINDOW)) + return false; + + fast_spi_get_ext_bios_window(base, size); + + /* Enable extended bios only if Size of Bios region is greater than 16MiB */ + if (*size == 0 || *base == 0) + return false; + + /* Round to power of two */ + alignment = 1UL << (log2_ceil(*size)); + *size = ALIGN_UP(*size, alignment); + *base = ALIGN_DOWN(*base, *size); + + return true; +} + +static void fast_spi_cache_ext_bios_window(void) +{ + size_t ext_bios_size; + uintptr_t ext_bios_base; + const int type = MTRR_TYPE_WRPROT; + + if (!fast_spi_ext_bios_cache_range(&ext_bios_base, &ext_bios_size)) + return; + + int mtrr = get_free_var_mtrr(); + if (mtrr == -1) + return; + set_var_mtrr(mtrr, ext_bios_base, ext_bios_size, type); +} + +void fast_spi_cache_ext_bios_postcar(struct postcar_frame *pcf) +{ + size_t ext_bios_size; + uintptr_t ext_bios_base; + const int type = MTRR_TYPE_WRPROT; + + if (!fast_spi_ext_bios_cache_range(&ext_bios_base, &ext_bios_size)) + return; + + postcar_frame_add_mtrr(pcf, ext_bios_base, ext_bios_size, type); +} + void fast_spi_cache_bios_region(void) { size_t bios_size; @@ -245,6 +293,9 @@
set_var_mtrr(mtrr, base, bios_size, type); } + + /* Check if caching is needed for extended bios region if supported */ + fast_spi_cache_ext_bios_window(); }
/* diff --git a/src/soc/intel/common/block/include/intelblocks/fast_spi.h b/src/soc/intel/common/block/include/intelblocks/fast_spi.h index 81437b6..e3ffa6a 100644 --- a/src/soc/intel/common/block/include/intelblocks/fast_spi.h +++ b/src/soc/intel/common/block/include/intelblocks/fast_spi.h @@ -83,5 +83,10 @@ * SOC function to get SPI-DMI Destination Id */ uint32_t soc_get_spi_dmi_destination_id(void); +/* + * Add MTRR for extended BIOS region(when supported) to postcar frame + */ +struct postcar_frame; +void fast_spi_cache_ext_bios_postcar(struct postcar_frame *pcf);
#endif /* SOC_INTEL_COMMON_BLOCK_FAST_SPI_H */ diff --git a/src/soc/intel/common/block/systemagent/memmap.c b/src/soc/intel/common/block/systemagent/memmap.c index 27870b0..86ca4e1 100644 --- a/src/soc/intel/common/block/systemagent/memmap.c +++ b/src/soc/intel/common/block/systemagent/memmap.c @@ -5,6 +5,7 @@ #include <console/console.h> #include <cpu/x86/mtrr.h> #include <cpu/x86/smm.h> +#include <intelblocks/fast_spi.h> #include <intelblocks/systemagent.h> #include <types.h>
@@ -71,4 +72,7 @@
/* Cache the TSEG region */ postcar_enable_tseg_cache(pcf); + + /* Cache the extended BIOS region if it is supported */ + fast_spi_cache_ext_bios_postcar(pcf); }
-
Furquan Shaikh (Code Review) -
Srinidhi N Kaushik (Code Review)
-
Tim Wawrzynczak (Code Review)