Change in coreboot[master]: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm
Michael Niewöhner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm
This change reworks the PRMRR size selection and implements an algorithm for PRMRR size selection which will be needed in a follow-up change to make the maximum PRMRR size user-selectable via Kconfig.
Currently, PRMRR size must be set in the devicetree, while the value has to be verified to be working with the CPU. The selection of a valid PRMRR size is now done by an algorithm that tries to find a supported PRMRR size within a possibly specified limit by looking at MSR_PRMRR_VALID_CONFIG and trying to select the highest possible value at runtime withing a possibly specified limit.
That (hopefully) prevents setting a wrong, unsupported static value.
The limit can be set via the devicetree setting PrmrrSize, that will be replaced by Kconfig in the follow-up change. When a too strict limit was set, coreboot will die, printing an error message.
Further, this change does some restructuring of memmap.c (for the listed platforms) to a) integrate the previously described functionality and b) to align common functionality for later move to the common section.
Before this reunification can happen, some more details about PRMRR, its influence on SGX-enabled and -disabled systems have to be investigated and understood. This is being worked on.
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I8a773362f8d09c8a33ff0dfac357edc6ea3564e0 --- M src/soc/intel/apollolake/chip.h M src/soc/intel/apollolake/memmap.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/memmap.c M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/include/intelblocks/cpulib.h M src/soc/intel/common/block/include/intelblocks/msr.h M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/memmap.c M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/memmap.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 15 files changed, 128 insertions(+), 62 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/35902/1
diff --git a/src/soc/intel/apollolake/chip.h b/src/soc/intel/apollolake/chip.h index 85cfff9..0e2a5f5 100644 --- a/src/soc/intel/apollolake/chip.h +++ b/src/soc/intel/apollolake/chip.h @@ -198,4 +198,6 @@
typedef struct soc_intel_apollolake_config config_t;
+size_t get_prmrr_size(void); + #endif /* _SOC_APOLLOLAKE_CHIP_H_ */ diff --git a/src/soc/intel/apollolake/memmap.c b/src/soc/intel/apollolake/memmap.c index 77711eb..9885724 100644 --- a/src/soc/intel/apollolake/memmap.c +++ b/src/soc/intel/apollolake/memmap.c @@ -22,11 +22,24 @@ #include <cpu/x86/mtrr.h> #include <cpu/x86/smm.h> #include <device/pci.h> +#include <intelblocks/cpulib.h> #include <soc/systemagent.h> #include <soc/pci_devs.h>
#include "chip.h"
+/* Calculate PRMRR size based on user input PRMRR size and config */ +size_t get_prmrr_size(void) +{ + size_t prmrr_size = 0; + const config_t *config = config_of_soc(); + + if (config->sgx_enable) + prmrr_size = get_max_prmrr_size(config->PrmrrSize); + + return prmrr_size; +} + void *cbmem_top(void) { const config_t *config; @@ -35,11 +48,12 @@ if (!CONFIG(SOC_INTEL_GLK)) return tolum;
- config = config_of_soc(); - - /* FSP allocates 2x PRMRR Size Memory for alignment */ - if (config->sgx_enable) - tolum -= config->PrmrrSize * 2; + /* + * FSP-M allocates 2x PRMRR Size Memory for alignment + * As long as the PRMRR base is not set and locked via MSRs, this region is just + * regular ram. Currently, PRMRR only gets enabled when SGX is enabled. + */ + tolum -= get_prmrr_size() * 2;
return tolum; } diff --git a/src/soc/intel/apollolake/romstage.c b/src/soc/intel/apollolake/romstage.c index 8418919..258f4ff 100644 --- a/src/soc/intel/apollolake/romstage.c +++ b/src/soc/intel/apollolake/romstage.c @@ -269,9 +269,7 @@ /* Only for GLK */ FSP_M_CONFIG *m_cfg = &mupd->FspmConfig;
- const config_t *config = config_of_soc(); - - m_cfg->PrmrrSize = config->PrmrrSize; + m_cfg->PrmrrSize = get_prmrr_size();
/* * CpuMemoryTest in FSP tests 0 to 1M of the RAM after MRC init. diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h index b937699..933f859 100644 --- a/src/soc/intel/cannonlake/chip.h +++ b/src/soc/intel/cannonlake/chip.h @@ -433,4 +433,6 @@
typedef struct soc_intel_cannonlake_config config_t;
+size_t get_prmrr_size(const struct soc_intel_cannonlake_config *config); + #endif diff --git a/src/soc/intel/cannonlake/memmap.c b/src/soc/intel/cannonlake/memmap.c index f3286cc..a4c9738 100644 --- a/src/soc/intel/cannonlake/memmap.c +++ b/src/soc/intel/cannonlake/memmap.c @@ -23,6 +23,7 @@ #include <device/device.h> #include <device/pci.h> #include <fsp/util.h> +#include <intelblocks/cpulib.h> #include <intelblocks/ebda.h> #include <intelblocks/systemagent.h> #include <soc/pci_devs.h> @@ -48,30 +49,32 @@ return imr_size; }
+/* Calculate PRMRR size based on user input PRMRR size and config */ +size_t get_prmrr_size(const struct soc_intel_cannonlake_config *config) +{ + size_t prmrr_size = 0; + + /* Allocate at least 1 MiB PRMRR memory for C6DRAM */ + if (config->enable_c6dram) + prmrr_size = 1*MiB; + + if (config->sgx_enable) + prmrr_size = get_max_prmrr_size(config->PrmrrSize); + + return prmrr_size; +} + /* Calculate PRMRR size based on user input PRMRR size and alignment */ -static size_t get_prmrr_size(uintptr_t dram_base, +static size_t get_aligned_prmrr_size(uintptr_t dram_base, const struct soc_intel_cannonlake_config *config) { uintptr_t prmrr_base = dram_base; size_t prmrr_size;
- prmrr_size = config->PrmrrSize; + prmrr_size = get_prmrr_size(config);
- /* Allocate PRMRR memory for C6DRAM */ - if (!prmrr_size) { - if (config->enable_c6dram) - prmrr_size = 1*MiB; - else - return 0; - } - - /* - * PRMRR Sizes that are > 1MB and < 32MB are - * not supported and will fail out. - */ - if ((prmrr_size > 1*MiB) && (prmrr_size < 32*MiB)) - die("PRMRR Sizes that are > 1MB and < 32MB are not" - "supported!\n"); + if (!prmrr_size) + return 0;
prmrr_base -= prmrr_size; if (prmrr_size >= 32*MiB) @@ -125,7 +128,7 @@ config = config_of(dev);
/* Get PRMRR size */ - reserve_mem_base -= get_prmrr_size(reserve_mem_base, config); + reserve_mem_base -= get_aligned_prmrr_size(reserve_mem_base, config);
/* Get Tracehub size */ reserve_mem_base -= get_imr_size(); diff --git a/src/soc/intel/cannonlake/romstage/fsp_params.c b/src/soc/intel/cannonlake/romstage/fsp_params.c index 3ba997d..4031382 100644 --- a/src/soc/intel/cannonlake/romstage/fsp_params.c +++ b/src/soc/intel/cannonlake/romstage/fsp_params.c @@ -48,8 +48,10 @@ mask |= (1 << i); } m_cfg->PcieRpEnableMask = mask; - m_cfg->PrmrrSize = config->PrmrrSize; + m_cfg->EnableC6Dram = config->enable_c6dram; + m_cfg->PrmrrSize = get_prmrr_size(config); + #if CONFIG(SOC_INTEL_COMETLAKE) m_cfg->SerialIoUartDebugControllerNumber = CONFIG_UART_FOR_CONSOLE; #else diff --git a/src/soc/intel/common/block/cpu/cpulib.c b/src/soc/intel/common/block/cpu/cpulib.c index 75e7112..008770e 100644 --- a/src/soc/intel/common/block/cpu/cpulib.c +++ b/src/soc/intel/common/block/cpu/cpulib.c @@ -320,3 +320,29 @@ (msr_t) {.lo = 0xffffffff, .hi = 0xffffffff}); } } + +size_t get_max_prmrr_size(size_t limit) +{ + msr_t msr; + int i; + size_t valid; + + msr = rdmsr(MSR_PRMRR_VALID_CONFIG); + + /* find the greatest valid value within a possibly specified limit */ + for (i = 8; i >= 0; i--) { + valid = msr.lo & (1<<i); + if ((!limit && valid) || (limit && valid <= limit)) + break; + else if (i == 0) + valid = 0; + } + + /* die if we could not find a valid size within the limit */ + if (!valid) + die("Unsupported PRMRR size limit!") + + printk(BIOS_DEBUG, "PRMRR size set to %li MiB\n", valid/MiB); + + return valid; +} diff --git a/src/soc/intel/common/block/include/intelblocks/cpulib.h b/src/soc/intel/common/block/include/intelblocks/cpulib.h index 8630fd1..a99e532 100644 --- a/src/soc/intel/common/block/include/intelblocks/cpulib.h +++ b/src/soc/intel/common/block/include/intelblocks/cpulib.h @@ -18,6 +18,7 @@ #define SOC_INTEL_COMMON_BLOCK_CPULIB_H
#include <stdint.h> +#include <stddef.h>
/* * Set PERF_CTL MSR (0x199) P_Req with @@ -161,4 +162,7 @@ /* Configure Machine Check Architecture support */ void mca_configure(void);
+/* Get the maximum valid PRMRR size with respect to the specified limits */ +size_t get_max_prmrr_size(size_t limit); + #endif /* SOC_INTEL_COMMON_BLOCK_CPULIB_H */ diff --git a/src/soc/intel/common/block/include/intelblocks/msr.h b/src/soc/intel/common/block/include/intelblocks/msr.h index 6fdf26e..fbc43e7 100644 --- a/src/soc/intel/common/block/include/intelblocks/msr.h +++ b/src/soc/intel/common/block/include/intelblocks/msr.h @@ -64,6 +64,7 @@ #define MSR_PRMRR_PHYS_MASK 0x1f5 #define PRMRR_PHYS_MASK_LOCK (1 << 10) #define PRMRR_PHYS_MASK_VALID (1 << 11) +#define MSR_PRMRR_VALID_CONFIG 0x1fb #define MSR_POWER_CTL 0x1fc #define POWER_CTL_C1E_MASK (1 << 1) #define MSR_EVICT_CTL 0x2e0 diff --git a/src/soc/intel/icelake/chip.h b/src/soc/intel/icelake/chip.h index fc9341c..47c49ec 100644 --- a/src/soc/intel/icelake/chip.h +++ b/src/soc/intel/icelake/chip.h @@ -281,4 +281,6 @@
typedef struct soc_intel_icelake_config config_t;
+size_t get_prmrr_size(const struct soc_intel_icelake_config *config); + #endif diff --git a/src/soc/intel/icelake/memmap.c b/src/soc/intel/icelake/memmap.c index 20c4e6f..b1e7d53 100644 --- a/src/soc/intel/icelake/memmap.c +++ b/src/soc/intel/icelake/memmap.c @@ -22,6 +22,7 @@ #include <device/device.h> #include <device/pci.h> #include <fsp/util.h> +#include <intelblocks/cpulib.h> #include <intelblocks/ebda.h> #include <intelblocks/systemagent.h> #include <soc/pci_devs.h> @@ -46,30 +47,32 @@ return imr_size; }
+/* Calculate PRMRR size based on user input PRMRR size and config */ +size_t get_prmrr_size(const struct soc_intel_icelake_config *config) +{ + size_t prmrr_size = 0; + + /* Allocate at least 1 MiB PRMRR memory for C6DRAM */ + if (config->enable_c6dram) + prmrr_size = 1*MiB; + + if (config->sgx_enable) + prmrr_size = get_max_prmrr_size(config->PrmrrSize); + + return prmrr_size; +} + /* Calculate PRMRR size based on user input PRMRR size and alignment */ -static size_t get_prmrr_size(uintptr_t dram_base, +static size_t get_aligned_prmrr_size(uintptr_t dram_base, const struct soc_intel_icelake_config *config) { uintptr_t prmrr_base = dram_base; size_t prmrr_size;
- prmrr_size = config->PrmrrSize; + prmrr_size = get_prmrr_size(config);
- /* Allocate PRMRR memory for C6DRAM */ - if (!prmrr_size) { - if (config->enable_c6dram) - prmrr_size = 1*MiB; - else - return 0; - } - - /* - * PRMRR Sizes that are > 1MB and < 32MB are - * not supported and will fail out. - */ - if ((prmrr_size > 1*MiB) && (prmrr_size < 32*MiB)) - die("PRMRR Sizes that are > 1MB and < 32MB are not" - "supported!\n"); + if (!prmrr_size) + return 0;
prmrr_base -= prmrr_size; if (prmrr_size >= 32*MiB) @@ -123,7 +126,7 @@ config = config_of(dev);
/* Get PRMRR size */ - reserve_mem_base -= get_prmrr_size(reserve_mem_base, config); + reserve_mem_base -= get_aligned_prmrr_size(reserve_mem_base, config);
/* Get Tracehub size */ reserve_mem_base -= get_imr_size(); diff --git a/src/soc/intel/icelake/romstage/fsp_params.c b/src/soc/intel/icelake/romstage/fsp_params.c index 5bf3421..d504d9e 100644 --- a/src/soc/intel/icelake/romstage/fsp_params.c +++ b/src/soc/intel/icelake/romstage/fsp_params.c @@ -60,8 +60,10 @@ mask |= (1 << i); } m_cfg->PcieRpEnableMask = mask; - m_cfg->PrmrrSize = config->PrmrrSize; + m_cfg->EnableC6Dram = config->enable_c6dram; + m_cfg->PrmrrSize = get_prmrr_size(config); + /* Disable BIOS Guard */ m_cfg->BiosGuard = 0; /* Disable Cpu Ratio Override temporary. */ diff --git a/src/soc/intel/skylake/chip.h b/src/soc/intel/skylake/chip.h index 944315b..fb92a5d 100644 --- a/src/soc/intel/skylake/chip.h +++ b/src/soc/intel/skylake/chip.h @@ -602,4 +602,6 @@
typedef struct soc_intel_skylake_config config_t;
+size_t get_prmrr_size(void); + #endif diff --git a/src/soc/intel/skylake/memmap.c b/src/soc/intel/skylake/memmap.c index 29f2517..0073cf7 100644 --- a/src/soc/intel/skylake/memmap.c +++ b/src/soc/intel/skylake/memmap.c @@ -23,6 +23,7 @@ #include <cpu/x86/smm.h> #include <device/device.h> #include <device/pci.h> +#include <intelblocks/cpulib.h> #include <intelblocks/ebda.h> #include <intelblocks/systemagent.h> #include <soc/msr.h> @@ -79,29 +80,32 @@ return tracehub_size; }
+/* Calculate PRMRR size based on user input PRMRR size and config */ +size_t get_prmrr_size(void) +{ + size_t prmrr_size = 0; + const config_t *config = config_of_soc(); + + if (CONFIG(PLATFORM_USES_FSP1_1)) + prmrr_size = 1*MiB; + + else if (config->sgx_enable) + prmrr_size = get_max_prmrr_size(config->PrmrrSize); + + return prmrr_size; +} + /* Calculate PRMRR size based on user input PRMRR size and alignment */ -static size_t get_prmrr_size(uintptr_t dram_base, - const struct soc_intel_skylake_config *config) +static size_t get_aligned_prmrr_size(uintptr_t dram_base) { uintptr_t prmrr_base = dram_base; size_t prmrr_size;
- if (CONFIG(PLATFORM_USES_FSP1_1)) - prmrr_size = 1*MiB; - else - prmrr_size = config->PrmrrSize; + prmrr_size = get_prmrr_size();
if (!prmrr_size) return 0;
- /* - * PRMRR Sizes that are > 1MB and < 32MB are - * not supported and will fail out. - */ - if ((prmrr_size > 1*MiB) && (prmrr_size < 32*MiB)) - die("PRMRR Sizes that are > 1MB and < 32MB are not" - "supported!\n"); - prmrr_base -= prmrr_size; if (prmrr_size >= 32*MiB) prmrr_base = ALIGN_DOWN(prmrr_base, 128*MiB); @@ -153,7 +157,7 @@ config = config_of(dev);
/* Get PRMRR size */ - reserve_mem_base -= get_prmrr_size(reserve_mem_base, config); + reserve_mem_base -= get_aligned_prmrr_size(reserve_mem_base);
/* Get Tracehub size */ reserve_mem_base -= get_tracehub_size(reserve_mem_base, config); diff --git a/src/soc/intel/skylake/romstage/romstage_fsp20.c b/src/soc/intel/skylake/romstage/romstage_fsp20.c index af89441..c7028b9 100644 --- a/src/soc/intel/skylake/romstage/romstage_fsp20.c +++ b/src/soc/intel/skylake/romstage/romstage_fsp20.c @@ -237,7 +237,8 @@ m_cfg->CmdTriStateDis = config->CmdTriStateDis; m_cfg->DdrFreqLimit = config->DdrFreqLimit; m_cfg->VmxEnable = CONFIG(ENABLE_VMX); - m_cfg->PrmrrSize = config->PrmrrSize; + m_cfg->PrmrrSize = get_prmrr_size(); + for (i = 0; i < ARRAY_SIZE(config->PcieRpEnable); i++) { if (config->PcieRpEnable[i]) mask |= (1<<i);
Hello Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35902
to look at the new patch set (#2).
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm
This change reworks the PRMRR size selection and implements an algorithm for PRMRR size selection which will be needed in a follow-up change to make the maximum PRMRR size user-selectable via Kconfig.
Currently, PRMRR size must be set in the devicetree, while the value has to be verified to be working with the CPU. The selection of a valid PRMRR size is now done by an algorithm that tries to find a supported PRMRR size within a possibly specified limit by looking at MSR_PRMRR_VALID_CONFIG and trying to select the highest possible value at runtime withing a possibly specified limit.
That (hopefully) prevents setting a wrong, unsupported static value.
The limit can be set via the devicetree setting PrmrrSize, that will be replaced by Kconfig in the follow-up change. When a too strict limit was set, coreboot will die, printing an error message.
Further, this change does some restructuring of memmap.c (for the listed platforms) to a) integrate the previously described functionality and b) to align common functionality for later move to the common section.
Before this reunification can happen, some more details about PRMRR, its influence on SGX-enabled and -disabled systems have to be investigated and understood. This is being worked on.
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I8a773362f8d09c8a33ff0dfac357edc6ea3564e0 --- M src/soc/intel/apollolake/chip.h M src/soc/intel/apollolake/memmap.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/memmap.c M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/include/intelblocks/cpulib.h M src/soc/intel/common/block/include/intelblocks/msr.h M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/memmap.c M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/memmap.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 15 files changed, 132 insertions(+), 65 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/35902/2
Hello Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35902
to look at the new patch set (#3).
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm
This change reworks the PRMRR size selection and implements an algorithm for PRMRR size selection which will be needed in a follow-up change to make the maximum PRMRR size user-selectable via Kconfig.
Currently, PRMRR size must be set in the devicetree, while the value has to be verified to be working with the CPU. The selection of a valid PRMRR size is now done by an algorithm that tries to find a supported PRMRR size within a possibly specified limit by looking at MSR_PRMRR_VALID_CONFIG and trying to select the highest possible value at runtime withing a possibly specified limit.
That (hopefully) prevents setting a wrong, unsupported static value.
The limit can be set via the devicetree setting PrmrrSize, that will be replaced by Kconfig in the follow-up change. When a too strict limit was set, coreboot will die, printing an error message.
Further, this change does some restructuring of memmap.c (for the listed platforms) to a) integrate the previously described functionality and b) to align common functionality for later move to the common section.
Before this reunification can happen, some more details about PRMRR, its influence on SGX-enabled and -disabled systems have to be investigated and understood. This is being worked on.
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I8a773362f8d09c8a33ff0dfac357edc6ea3564e0 --- M src/soc/intel/apollolake/chip.h M src/soc/intel/apollolake/memmap.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/memmap.c M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/include/intelblocks/cpulib.h M src/soc/intel/common/block/include/intelblocks/msr.h M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/memmap.c M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/memmap.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 15 files changed, 130 insertions(+), 65 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/35902/3
Hello Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35902
to look at the new patch set (#4).
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm
This change reworks the PRMRR size selection and implements an algorithm for PRMRR size selection which will be needed in a follow-up change to make the maximum PRMRR size user-selectable via Kconfig.
Currently, PRMRR size must be set in the devicetree, while the value has to be verified to be working with the CPU. The selection of a valid PRMRR size is now done by an algorithm that tries to find a supported PRMRR size within a possibly specified limit by looking at MSR_PRMRR_VALID_CONFIG and trying to select the highest possible value at runtime withing a possibly specified limit.
That (hopefully) prevents setting a wrong, unsupported static value.
The limit can be set via the devicetree setting PrmrrSize, that will be replaced by Kconfig in the follow-up change. When a too strict limit was set, coreboot will die, printing an error message.
Further, this change does some restructuring of memmap.c (for the listed platforms) to a) integrate the previously described functionality and b) to align common functionality for later move to the common section.
Before this reunification can happen, some more details about PRMRR, its influence on SGX-enabled and -disabled systems have to be investigated and understood. This is being worked on.
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I8a773362f8d09c8a33ff0dfac357edc6ea3564e0 --- M src/soc/intel/apollolake/chip.h M src/soc/intel/apollolake/memmap.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/memmap.c M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/include/intelblocks/cpulib.h M src/soc/intel/common/block/include/intelblocks/msr.h M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/memmap.c M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/memmap.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 15 files changed, 130 insertions(+), 65 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/35902/4
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Patch Set 4:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35902/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35902/4//COMMIT_MSG@18 PS4, Line 18: withing within
https://review.coreboot.org/c/coreboot/+/35902/4//COMMIT_MSG@23 PS4, Line 23: was is
https://review.coreboot.org/c/coreboot/+/35902/4//COMMIT_MSG@33 PS4, Line 33: Tested how?
https://review.coreboot.org/c/coreboot/+/35902/4/src/soc/intel/cannonlake/me... File src/soc/intel/cannonlake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35902/4/src/soc/intel/cannonlake/me... PS4, Line 60: * Please add spaces around the operator.
https://review.coreboot.org/c/coreboot/+/35902/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35902/4/src/soc/intel/common/block/... PS4, Line 328: valid valid sounds like a boolean to me. Maybe there is an other name? valid_size?
https://review.coreboot.org/c/coreboot/+/35902/4/src/soc/intel/common/block/... PS4, Line 334: << Please add spaces around the operator.
https://review.coreboot.org/c/coreboot/+/35902/4/src/soc/intel/common/block/... PS4, Line 343: die("Unsupported PRMRR size limit!"); Add more information? Look in the devicetree or something similar? Also print the value of `MSR_PRMRR_VALID_CONFIG`?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Uploaded patch set 5.
(4 comments)
https://review.coreboot.org/c/coreboot/+/35902/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35902/4//COMMIT_MSG@33 PS4, Line 33:
Tested how?
did some short tests yet, but needs definitely excessive testing; will be added as soon as this happended
https://review.coreboot.org/c/coreboot/+/35902/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35902/4/src/soc/intel/common/block/... PS4, Line 328: valid
valid sounds like a boolean to me. […]
one should never derive the datatype from a variable name IMHO, but valid_size is ok for me, will change it
https://review.coreboot.org/c/coreboot/+/35902/4/src/soc/intel/common/block/... PS4, Line 334: <<
Please add spaces around the operator.
not convinced. I would aggree for any other operator, but not for shift. What do the others say?
https://review.coreboot.org/c/coreboot/+/35902/4/src/soc/intel/common/block/... PS4, Line 343: die("Unsupported PRMRR size limit!");
Add more information? Look in the devicetree or something similar? Also print the value of `MSR_PRMR […]
Done
Hello Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35902
to look at the new patch set (#5).
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm
This change reworks the PRMRR size selection and implements an algorithm for PRMRR size selection which will be needed in a follow-up change to make the maximum PRMRR size user-selectable via Kconfig.
Currently, PRMRR size must be set in the devicetree, while the value has to be verified to be working with the CPU. The selection of a valid PRMRR size is now done by an algorithm that tries to find a supported PRMRR size within a possibly specified limit by looking at MSR_PRMRR_VALID_CONFIG and trying to select the highest possible value at runtime within a possibly specified limit.
That (hopefully) prevents setting a wrong, unsupported static value.
The limit can be set via the devicetree setting PrmrrSize, that will be replaced by Kconfig in the follow-up change. When a too strict limit is set, coreboot will die, printing an error message.
Further, this change does some restructuring of memmap.c (for the listed platforms) to a) integrate the previously described functionality and b) to align common functionality for later move to the common section.
Before this reunification can happen, some more details about PRMRR, its influence on SGX-enabled and -disabled systems have to be investigated and understood. This is being worked on.
Todo: needs testing
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I8a773362f8d09c8a33ff0dfac357edc6ea3564e0 --- M src/soc/intel/apollolake/chip.h M src/soc/intel/apollolake/memmap.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/memmap.c M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/include/intelblocks/cpulib.h M src/soc/intel/common/block/include/intelblocks/msr.h M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/memmap.c M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/memmap.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 15 files changed, 133 insertions(+), 65 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/35902/5
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35902/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35902/4//COMMIT_MSG@18 PS4, Line 18: withing
within
Done
https://review.coreboot.org/c/coreboot/+/35902/4//COMMIT_MSG@23 PS4, Line 23: was
is
Done
https://review.coreboot.org/c/coreboot/+/35902/4/src/soc/intel/cannonlake/me... File src/soc/intel/cannonlake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35902/4/src/soc/intel/cannonlake/me... PS4, Line 60: *
Please add spaces around the operator.
Done
Hello Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35902
to look at the new patch set (#6).
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm
This change reworks the PRMRR size selection and implements an algorithm for PRMRR size selection which will be needed in a follow-up change to make the maximum PRMRR size user-selectable via Kconfig.
Currently, PRMRR size must be set in the devicetree, while the value has to be verified to be working with the CPU. The selection of a valid PRMRR size is now done by an algorithm that tries to find a supported PRMRR size within a possibly specified limit by looking at MSR_PRMRR_VALID_CONFIG and trying to select the highest possible value at runtime within a possibly specified limit.
That (hopefully) prevents setting a wrong, unsupported static value.
The limit can be set via the devicetree setting PrmrrSize, that will be replaced by Kconfig in the follow-up change. When a too strict limit is set, coreboot will die, printing an error message.
Further, this change does some restructuring of memmap.c (for the listed platforms) to a) integrate the previously described functionality and b) to align common functionality for later move to the common section.
Before this reunification can happen, some more details about PRMRR, its influence on SGX-enabled and -disabled systems have to be investigated and understood. This is being worked on.
Todo: needs testing
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I8a773362f8d09c8a33ff0dfac357edc6ea3564e0 --- M src/soc/intel/apollolake/chip.h M src/soc/intel/apollolake/memmap.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/memmap.c M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/include/intelblocks/cpulib.h M src/soc/intel/common/block/include/intelblocks/msr.h M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/memmap.c M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/memmap.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 15 files changed, 141 insertions(+), 73 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/35902/6
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Uploaded patch set 6.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Uploaded patch set 7.
Hello Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35902
to look at the new patch set (#7).
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm
This change reworks the PRMRR size selection and implements an algorithm for PRMRR size selection which will be needed in a follow-up change to make the maximum PRMRR size user-selectable via Kconfig.
Currently, PRMRR size must be set in the devicetree, while the value has to be verified to be working with the CPU. The selection of a valid PRMRR size is now done by an algorithm that tries to find a supported PRMRR size within a possibly specified limit by looking at MSR_PRMRR_VALID_CONFIG and trying to select the highest possible value at runtime within a possibly specified limit.
That (hopefully) prevents setting a wrong, unsupported static value.
The limit can be set via the devicetree setting PrmrrSize, that will be replaced by Kconfig in the follow-up change. When a too strict limit is set, coreboot will die, printing an error message.
Further, this change does some restructuring of memmap.c (for the listed platforms) to a) integrate the previously described functionality and b) to align common functionality for later move to the common section.
Before this reunification can happen, some more details about PRMRR, its influence on SGX-enabled and -disabled systems have to be investigated and understood. This is being worked on.
Todo: needs testing
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I8a773362f8d09c8a33ff0dfac357edc6ea3564e0 --- M src/soc/intel/apollolake/chip.h M src/soc/intel/apollolake/memmap.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/memmap.c M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/include/intelblocks/cpulib.h M src/soc/intel/common/block/include/intelblocks/msr.h M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/memmap.c M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/memmap.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 15 files changed, 141 insertions(+), 73 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/35902/7
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35902/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35902/7//COMMIT_MSG@34 PS7, Line 34: Todo: needs testing kindly help to test on CNL and ICL variants ?
Also one question, prmrr algorithm need to establish in same way how its been done inside FSP-M else size calculation might cause issue. How do you managed to do that ?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35902/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35902/7//COMMIT_MSG@34 PS7, Line 34: Todo: needs testing
kindly help to test on CNL and ICL variants ? […]
Testers are welcome! :-) Well, currently nobody knows, what FSP-M does... this was before my CR and will be the same after. I am currently reverse engineering PRMRR related stuff to get more details but that will take some time...
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Patch Set 8:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35902/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35902/7//COMMIT_MSG@34 PS7, Line 34: Todo: needs testing
Testers are welcome! :-) Well, currently nobody knows, what FSP-M does... […]
I think all we have to do is to give FSP-M a size setting that the CPU supports. Then we can be sure that it will use exactly that setting.
Then, the only thing we have to know about FSP-M is how it will align PRMRR. (And only if we keep mimicking the whole FSP memmap. As we use the EBDA to cache TOLUD now, I assume we could also get rid of that and store the address returned by FSP instead.)
Subrata, in the coreboot code we currently have 128MiB alignment for the big cores and no alignment but 2x the configured size for GLK. Can you confirm the alignment, please? I don't have access to all the sources. A grep for `MAX_PRMRR` should help to find the respective code.
I'm rather sure the alignment is 256MiB for newer SoCs. Maybe Kaby Lake is the exception, though, and really aligns to 128MiB.
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/apollolake/ch... File src/soc/intel/apollolake/chip.h:
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/apollolake/ch... PS8, Line 201: size_t get_prmrr_size(void); `chip.h` is dedicated to the devicetree configuration. A better place would be one of the include/soc/ headers. I can't find any good match, though, maybe add a `include/soc/memmap.h`?
If you intend to call this function from common code, it should be prefixed with `soc_`.
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/cannonlake/me... File src/soc/intel/cannonlake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/cannonlake/me... PS8, Line 83: prmrr_base = ALIGN_DOWN(prmrr_base, 16 * MiB); This case needs further analysis. The last code I've seen seems to try to fill the gap for TSEG alignment, but doesn't explicitly align down. As no board sets `enable_c6dram`, I assume this case is untested.
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/common/block/... PS8, Line 346: MiB As far as I followed the code, it's in MiB already.
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/common/block/... PS8, Line 346: valid_size limit?
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/common/block/... PS8, Line 348: MiB As far as I followed the code, it's in MiB already.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/common/block/... PS8, Line 346: valid_size
limit?
limit will be used to set the maximum size later in Kconfig, see sgx Kconfig commit. We could remove this but then we have no fallback when a user sets a value that is not supported
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/cannonlake/me... File src/soc/intel/cannonlake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/cannonlake/me... PS8, Line 83: prmrr_base = ALIGN_DOWN(prmrr_base, 16 * MiB);
This case needs further analysis. The last code I've seen seems […]
Ok, do you have a board for testing? I have none supporting c6dram here
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/cannonlake/me... File src/soc/intel/cannonlake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/cannonlake/me... PS8, Line 83: prmrr_base = ALIGN_DOWN(prmrr_base, 16 * MiB);
Ok, do you have a board for testing? I have none supporting c6dram here
We should tested on CNL and ICL based platform where C6 DRAM been supported, i'm currently OOO till 23th if you could wait, i can plan for verifying this CL
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35902/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35902/7//COMMIT_MSG@34 PS7, Line 34: Todo: needs testing
I think all we have to do is to give FSP-M a size setting that the CPU supports. Then we can be sure that it will use exactly that setting.
Yes, this is what I expect, as this is exactly what we are doing currently with that ugly devicetree setting.
Then, the only thing we have to know about FSP-M is how it will align PRMRR. (And only if we keep mimicking the whole FSP memmap.
As we use the EBDA to cache TOLUD now, I assume we could also get rid of that and store the address returned by FSP instead.)
If I am not mistaken, this would even allow us to get rid of the user limit I implemented, as we simply set the Kconfig value, the user selected, and then don't need to do any calculation here.
Subrata, in the coreboot code we currently have 128MiB alignment for the big cores and no alignment but 2x the configured size for GLK. Can you confirm the alignment, please? I don't have access to all the sources. A grep for `MAX_PRMRR` should help to find the respective code.
I'm rather sure the alignment is 256MiB for newer SoCs. Maybe Kaby Lake is the exception, though, and really aligns to 128MiB.
For me it seems the maximum possible value is used for alignedment. KBL does not support 256, so aligment happens to 128. Not sure for the previous platforms, though.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35902/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35902/7//COMMIT_MSG@34 PS7, Line 34: Todo: needs testing
I'm rather sure the alignment is 256MiB for newer SoCs. Maybe Kaby Lake is the exception, though, and really aligns to 128MiB.
For me it seems the maximum possible value is used for alignedment. KBL does not support 256, so aligment happens to 128. Not sure for the previous platforms, though.
Do you mean KBL or its FSP doesn't support 256MiB? The former seems to support it. At least IIRC, you reported a 0x1e0 for your MSR reading? Wasn't that KBL?
If the CPU supports it, there's a 50% chance that we can test the latter. Just set it to 256MiB and see where it is aligned (if 128/256MiB align- ment would mean the same, that says nothing, though).
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/common/block/... PS8, Line 346: valid_size
limit will be used to set the maximum size later in Kconfig, see sgx Kconfig commit. […]
I meant you use the wrong variable. The text says unsupported limit, you output constant 0 (in form of the variable `valid_size`).
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Aamir Bohra, Wonkyu Kim, Maulik V Vaghela, Paul Menzel, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Srinidhi N Kaushik, Lean Sheng Tan, V Sowmya, Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35902
to look at the new patch set (#9).
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm
This change reworks the PRMRR size selection and implements an algorithm for PRMRR size selection which will be needed in a follow-up change to make the maximum PRMRR size user-selectable via Kconfig.
Currently, PRMRR size must be set in the devicetree, while the value has to be verified to be working with the CPU. The selection of a valid PRMRR size is now done by an algorithm that tries to find a supported PRMRR size within a possibly specified limit by looking at MSR_PRMRR_VALID_CONFIG and trying to select the highest possible value at runtime within a possibly specified limit.
That (hopefully) prevents setting a wrong, unsupported static value.
The limit can be set via the devicetree setting PrmrrSize, that will be replaced by Kconfig in the follow-up change. When a too strict limit is set, coreboot will die, printing an error message.
Further, this change does some restructuring of memmap.c (for the listed platforms) to a) integrate the previously described functionality and b) to align common functionality for later move to the common section.
Before this reunification can happen, some more details about PRMRR, its influence on SGX-enabled and -disabled systems have to be investigated and understood. This is being worked on.
Todo: needs testing
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I8a773362f8d09c8a33ff0dfac357edc6ea3564e0 --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb M src/mainboard/intel/icelake_rvp/variants/icl_u/devicetree.cb M src/mainboard/intel/icelake_rvp/variants/icl_y/devicetree.cb M src/mainboard/supermicro/x11-lga1151-series/devicetree.cb M src/soc/intel/apollolake/chip.h A src/soc/intel/apollolake/include/soc/memmap.h M src/soc/intel/apollolake/memmap.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/chip.h A src/soc/intel/cannonlake/include/soc/memmap.h M src/soc/intel/cannonlake/memmap.c M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/include/intelblocks/cpulib.h M src/soc/intel/common/block/include/intelblocks/msr.h M src/soc/intel/icelake/chip.h A src/soc/intel/icelake/include/soc/memmap.h M src/soc/intel/icelake/memmap.c M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/chip.h A src/soc/intel/skylake/include/soc/memmap.h M src/soc/intel/skylake/memmap.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 24 files changed, 241 insertions(+), 91 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/35902/9
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Uploaded patch set 9.
(3 comments)
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/common/block/... PS8, Line 346: valid_size
I meant you use the wrong variable. The text says unsupported limit, you […]
Ahh, yes of course, thank you
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/common/block/... PS8, Line 346: MiB
As far as I followed the code, it's in MiB already.
Done
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/common/block/... PS8, Line 348: MiB
As far as I followed the code, it's in MiB already.
Done
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Aamir Bohra, Wonkyu Kim, Maulik V Vaghela, Paul Menzel, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Srinidhi N Kaushik, Lean Sheng Tan, V Sowmya, Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35902
to look at the new patch set (#10).
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm
This change reworks the PRMRR size selection and implements an algorithm for PRMRR size selection which will be needed in a follow-up change to make the maximum PRMRR size user-selectable via Kconfig.
Currently, PRMRR size must be set in the devicetree, while the value has to be verified to be working with the CPU. The selection of a valid PRMRR size is now done by an algorithm that tries to find a supported PRMRR size within a possibly specified limit by looking at MSR_PRMRR_VALID_CONFIG and trying to select the highest possible value at runtime within a possibly specified limit.
That (hopefully) prevents setting a wrong, unsupported static value.
The limit can be set via the devicetree setting PrmrrSize, that will be replaced by Kconfig in the follow-up change. When a too strict limit is set, coreboot will die, printing an error message.
Further, this change does some restructuring of memmap.c (for the listed platforms) to a) integrate the previously described functionality and b) to align common functionality for later move to the common section.
Before this reunification can happen, some more details about PRMRR, its influence on SGX-enabled and -disabled systems have to be investigated and understood. This is being worked on.
Todo: needs testing
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I8a773362f8d09c8a33ff0dfac357edc6ea3564e0 --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb M src/mainboard/intel/icelake_rvp/variants/icl_u/devicetree.cb M src/mainboard/intel/icelake_rvp/variants/icl_y/devicetree.cb M src/mainboard/supermicro/x11-lga1151-series/devicetree.cb M src/soc/intel/apollolake/Makefile.inc M src/soc/intel/apollolake/chip.h A src/soc/intel/apollolake/include/soc/memmap.h M src/soc/intel/apollolake/memmap.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/chip.h A src/soc/intel/cannonlake/include/soc/memmap.h M src/soc/intel/cannonlake/memmap.c M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/include/intelblocks/cpulib.h M src/soc/intel/common/block/include/intelblocks/msr.h M src/soc/intel/icelake/chip.h A src/soc/intel/icelake/include/soc/memmap.h M src/soc/intel/icelake/memmap.c M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/chip.h A src/soc/intel/skylake/include/soc/memmap.h M src/soc/intel/skylake/memmap.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 25 files changed, 242 insertions(+), 91 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/35902/10
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Uploaded patch set 10.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35902/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35902/4//COMMIT_MSG@33 PS4, Line 33:
did some short tests yet, but needs definitely excessive testing; will be added as soon as this happ […]
added todo
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/apollolake/ch... File src/soc/intel/apollolake/chip.h:
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/apollolake/ch... PS8, Line 201: size_t get_prmrr_size(void);
`chip.h` is dedicated to the devicetree configuration. A better place […]
Done
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Aamir Bohra, Wonkyu Kim, Maulik V Vaghela, Paul Menzel, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Srinidhi N Kaushik, Lean Sheng Tan, V Sowmya, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35902
to look at the new patch set (#11).
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm
This change reworks the PRMRR size selection and implements an algorithm for PRMRR size selection which will be needed in a follow-up change to make the maximum PRMRR size user-selectable via Kconfig.
Currently, PRMRR size must be set in the devicetree, while the value has to be verified to be working with the CPU. The selection of a valid PRMRR size is now done by an algorithm that tries to find a supported PRMRR size within a possibly specified limit by looking at MSR_PRMRR_VALID_CONFIG and trying to select the highest possible value at runtime within a possibly specified limit.
The limit can be set via the devicetree setting PrmrrSize, that will be replaced by Kconfig in the follow-up change. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I8a773362f8d09c8a33ff0dfac357edc6ea3564e0 --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb M src/mainboard/intel/icelake_rvp/variants/icl_u/devicetree.cb M src/mainboard/intel/icelake_rvp/variants/icl_y/devicetree.cb M src/mainboard/supermicro/x11-lga1151-series/devicetree.cb M src/soc/intel/apollolake/Makefile.inc M src/soc/intel/apollolake/chip.h M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/include/intelblocks/cpulib.h M src/soc/intel/common/block/include/intelblocks/msr.h M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 17 files changed, 62 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/35902/11
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Uploaded patch set 11.
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Aamir Bohra, Wonkyu Kim, Maulik V Vaghela, Paul Menzel, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Srinidhi N Kaushik, Lean Sheng Tan, V Sowmya, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35902
to look at the new patch set (#12).
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm
This change reworks the PRMRR size selection and implements an algorithm for PRMRR size selection which will be needed in a follow-up change to make the maximum PRMRR size user-selectable via Kconfig.
Currently, PRMRR size must be set in the devicetree, while the value has to be verified to be working with the CPU. The selection of a valid PRMRR size is now done by an algorithm that tries to find a supported PRMRR size within a possibly specified limit by looking at MSR_PRMRR_VALID_CONFIG and trying to select the highest possible value at runtime within a possibly specified limit.
The limit can be set via the devicetree setting PrmrrSize, that will be replaced by Kconfig in the follow-up change. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I8a773362f8d09c8a33ff0dfac357edc6ea3564e0 --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb M src/mainboard/intel/icelake_rvp/variants/icl_u/devicetree.cb M src/mainboard/intel/icelake_rvp/variants/icl_y/devicetree.cb M src/mainboard/supermicro/x11-lga1151-series/devicetree.cb M src/soc/intel/apollolake/Makefile.inc M src/soc/intel/apollolake/chip.h M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/include/intelblocks/cpulib.h M src/soc/intel/common/block/include/intelblocks/msr.h M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 17 files changed, 62 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/35902/12
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Uploaded patch set 13: Patch Set 12 was rebased.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Patch Set 13:
(2 comments)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/35902/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35902/7//COMMIT_MSG@34 PS7, Line 34: Todo: needs testing
I'm rather sure the alignment is 256MiB for newer SoCs. Maybe […]
The msr was 0xe1, which is 128-32 MiB and 1 MiB; I tested, 256 which leads to a hang on payload call
https://review.coreboot.org/c/coreboot/+/35902/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35902/4/src/soc/intel/common/block/... PS4, Line 334: <<
not convinced. I would aggree for any other operator, but not for shift. […]
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Patch Set 13:
(2 comments)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/35902/13/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35902/13/src/soc/intel/common/block... PS13, Line 345: die("Unsupported PRMRR size limit %i MiB, check your config!\n", limit); What if PRMRR isn't supported at all? What does this MSR read back? I'm curious if this MSR always returns values that are non-zero. If not, we'll die() here.
https://review.coreboot.org/c/coreboot/+/35902/13/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cpulib.h:
https://review.coreboot.org/c/coreboot/+/35902/13/src/soc/intel/common/block... PS13, Line 165: /* Get the maximum valid PRMRR size with respect to the specified limits */ Indicate the units returned. Code inspection seems to indicate 'size in bytes'.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Uploaded patch set 14: Patch Set 13 was rebased.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Uploaded patch set 15: Patch Set 14 was rebased.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Uploaded patch set 16.
(5 comments)
https://review.coreboot.org/c/coreboot/+/35902/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35902/7//COMMIT_MSG@34 PS7, Line 34: Todo: needs testing
Thanks for checking. Must have twisted e1 <-> 1e. […]
exactly :)
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/cannonlake/me... File src/soc/intel/cannonlake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/cannonlake/me... PS8, Line 83: prmrr_base = ALIGN_DOWN(prmrr_base, 16 * MiB);
We should tested on CNL and ICL based platform where C6 DRAM been supported, i'm currently OOO till […]
outdated
https://review.coreboot.org/c/coreboot/+/35902/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35902/4/src/soc/intel/common/block/... PS4, Line 334: <<
The rule is simple, we place spaces around all binary operators. […]
Done
https://review.coreboot.org/c/coreboot/+/35902/13/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35902/13/src/soc/intel/common/block... PS13, Line 345: die("Unsupported PRMRR size limit %i MiB, check your config!\n", limit);
What if PRMRR isn't supported at all? What does this MSR read back? I'm curious if this MSR always r […]
Well, AFAIK prmrr is supported on any cpu, but was named EMRR on previous families
https://review.coreboot.org/c/coreboot/+/35902/13/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cpulib.h:
https://review.coreboot.org/c/coreboot/+/35902/13/src/soc/intel/common/block... PS13, Line 165: /* Get the maximum valid PRMRR size with respect to the specified limits */
Indicate the units returned. Code inspection seems to indicate 'size in bytes'.
Done
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Aamir Bohra, Wonkyu Kim, Maulik V Vaghela, Paul Menzel, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Srinidhi N Kaushik, Lean Sheng Tan, V Sowmya, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35902
to look at the new patch set (#16).
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm
This change reworks the PRMRR size selection and implements an algorithm for PRMRR size selection which will be needed in a follow-up change to make the maximum PRMRR size user-selectable via Kconfig.
Currently, PRMRR size must be set in the devicetree, while the value has to be verified to be working with the CPU. The selection of a valid PRMRR size is now done by an algorithm that tries to find a supported PRMRR size within a possibly specified limit by looking at MSR_PRMRR_VALID_CONFIG and trying to select the highest possible value at runtime within a possibly specified limit.
The limit can be set via the devicetree setting PrmrrSize, that will be replaced by Kconfig in the follow-up change. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I8a773362f8d09c8a33ff0dfac357edc6ea3564e0 --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb M src/mainboard/intel/icelake_rvp/variants/icl_u/devicetree.cb M src/mainboard/intel/icelake_rvp/variants/icl_y/devicetree.cb M src/mainboard/supermicro/x11-lga1151-series/devicetree.cb M src/soc/intel/apollolake/Makefile.inc M src/soc/intel/apollolake/chip.h M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/include/intelblocks/cpulib.h M src/soc/intel/common/block/include/intelblocks/msr.h M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 17 files changed, 62 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/35902/16
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35902/13/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35902/13/src/soc/intel/common/block... PS13, Line 345: die("Unsupported PRMRR size limit %i MiB, check your config!\n", limit);
Well, AFAIK prmrr is supported on any cpu, but was named EMRR on previous families
I'm not worried about the names. I was wondering about the coupling of the msr register to a feature actually being enabled. My concern was about the die() when we hit that case. If msr.lo is all 0s there's no point in die()ing. It should just return 0.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35902/13/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35902/13/src/soc/intel/common/block... PS13, Line 345: die("Unsupported PRMRR size limit %i MiB, check your config!\n", limit);
I'm not worried about the names. […]
Ah, now I got it; I don't know if there is any CPU out there that would return 0 but it doesn't hurt to continue here if that was the case. I'll add this
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Uploaded patch set 17: Patch Set 16 was rebased.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Uploaded patch set 19: Patch Set 18 was rebased.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Uploaded patch set 20.
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Aamir Bohra, Wonkyu Kim, Maulik V Vaghela, Paul Menzel, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Srinidhi N Kaushik, Lean Sheng Tan, V Sowmya, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35902
to look at the new patch set (#20).
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm
This change reworks the PRMRR size selection and implements an algorithm for PRMRR size selection which will be needed in a follow-up change to make the maximum PRMRR size user-selectable via Kconfig.
Currently, PRMRR size must be set in the devicetree, while the value has to be verified to be working with the CPU. The selection of a valid PRMRR size is now done by an algorithm that tries to find a supported PRMRR size within a possibly specified limit by looking at MSR_PRMRR_VALID_CONFIG and trying to select the highest possible value at runtime within a possibly specified limit.
The limit can be set via the devicetree setting PrmrrSize, that will be replaced by Kconfig in the follow-up change. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I8a773362f8d09c8a33ff0dfac357edc6ea3564e0 --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb M src/mainboard/intel/icelake_rvp/variants/icl_u/devicetree.cb M src/mainboard/intel/icelake_rvp/variants/icl_y/devicetree.cb M src/mainboard/supermicro/x11-lga1151-series/devicetree.cb M src/soc/intel/apollolake/Makefile.inc M src/soc/intel/apollolake/chip.h M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/include/intelblocks/cpulib.h M src/soc/intel/common/block/include/intelblocks/msr.h M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 17 files changed, 67 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/35902/20
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Uploaded patch set 21.
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Aamir Bohra, Wonkyu Kim, Maulik V Vaghela, Paul Menzel, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Srinidhi N Kaushik, Lean Sheng Tan, V Sowmya, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35902
to look at the new patch set (#21).
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm
This change reworks the PRMRR size selection and implements an algorithm for PRMRR size selection which will be needed in a follow-up change to make the maximum PRMRR size user-selectable via Kconfig.
Currently, PRMRR size must be set in the devicetree, while the value has to be verified to be working with the CPU. The selection of a valid PRMRR size is now done by an algorithm that tries to find a supported PRMRR size within a possibly specified limit by looking at MSR_PRMRR_VALID_CONFIG and trying to select the highest possible value at runtime within a possibly specified limit.
The limit can be set via the devicetree setting PrmrrSize, that will be replaced by Kconfig in the follow-up change. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I8a773362f8d09c8a33ff0dfac357edc6ea3564e0 --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb M src/mainboard/intel/icelake_rvp/variants/icl_u/devicetree.cb M src/mainboard/intel/icelake_rvp/variants/icl_y/devicetree.cb M src/mainboard/supermicro/x11-lga1151-series/devicetree.cb M src/soc/intel/apollolake/Makefile.inc M src/soc/intel/apollolake/chip.h M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/include/intelblocks/cpulib.h M src/soc/intel/common/block/include/intelblocks/msr.h M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 17 files changed, 67 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/35902/21
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Aamir Bohra, Wonkyu Kim, Maulik V Vaghela, Paul Menzel, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Srinidhi N Kaushik, Lean Sheng Tan, V Sowmya, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35902
to look at the new patch set (#22).
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm
This change reworks the PRMRR size selection and implements an algorithm for PRMRR size selection which will be needed in a follow-up change to make the maximum PRMRR size user-selectable via Kconfig.
Currently, PRMRR size must be set in the devicetree, while the value has to be verified to be working with the CPU. The selection of a valid PRMRR size is now done by an algorithm that tries to find a supported PRMRR size within a possibly specified limit by looking at MSR_PRMRR_VALID_CONFIG and trying to select the highest possible value at runtime within a possibly specified limit.
The limit can be set via the devicetree setting PrmrrSize, that will be replaced by Kconfig in the follow-up change. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I8a773362f8d09c8a33ff0dfac357edc6ea3564e0 --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb M src/mainboard/intel/icelake_rvp/variants/icl_u/devicetree.cb M src/mainboard/intel/icelake_rvp/variants/icl_y/devicetree.cb M src/mainboard/supermicro/x11-lga1151-series/devicetree.cb M src/soc/intel/apollolake/Makefile.inc M src/soc/intel/apollolake/chip.h M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/include/intelblocks/cpulib.h M src/soc/intel/common/block/include/intelblocks/msr.h M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 17 files changed, 73 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/35902/22
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Uploaded patch set 22.
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Aamir Bohra, Wonkyu Kim, Maulik V Vaghela, Paul Menzel, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Srinidhi N Kaushik, Lean Sheng Tan, V Sowmya, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35902
to look at the new patch set (#23).
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm
This change reworks the PRMRR size selection and implements an algorithm for PRMRR size selection which will be needed in a follow-up change to make the maximum PRMRR size user-selectable via Kconfig.
Currently, PRMRR size must be set in the devicetree, while the value has to be verified to be working with the CPU. The selection of a valid PRMRR size is now done by an algorithm that tries to find a supported PRMRR size within a possibly specified limit by looking at MSR_PRMRR_VALID_CONFIG and trying to select the highest possible value at runtime within a possibly specified limit.
The limit can be set via the devicetree setting PrmrrSize, that will be replaced by Kconfig in the follow-up change. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I8a773362f8d09c8a33ff0dfac357edc6ea3564e0 --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb M src/mainboard/intel/icelake_rvp/variants/icl_u/devicetree.cb M src/mainboard/intel/icelake_rvp/variants/icl_y/devicetree.cb M src/mainboard/supermicro/x11-lga1151-series/devicetree.cb M src/soc/intel/apollolake/Makefile.inc M src/soc/intel/apollolake/chip.h M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/include/intelblocks/cpulib.h M src/soc/intel/common/block/include/intelblocks/msr.h M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/romstage/romstage.c 17 files changed, 73 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/35902/23
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Uploaded patch set 23.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Patch Set 23:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35902/23/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35902/23/src/soc/intel/common/block... PS23, Line 337: } Move this up above the msr read?
https://review.coreboot.org/c/coreboot/+/35902/23/src/soc/intel/common/block... PS23, Line 348: limit == -1 Do we need to handle the unlimited case given that you are trying to drive everything from Kconfig?
I think we should just have:
int get_prmrr_size(void) where this function is the helper that handles your Kconfig translation parsing.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Patch Set 23:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35902/23/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35902/23/src/soc/intel/common/block... PS23, Line 348: limit == -1
Do we need to handle the unlimited case given that you are trying to drive everything from Kconfig? […]
Aaaaah! Now I got what you mean... I'm sorry, yes ofc. Will update this again, thank you!
Michael Niewöhner has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel: apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Abandoned
completely reworked in CB:35799
-
Aaron Durbin (Code Review) -
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)
-
Subrata Banik (Code Review)