Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45345 )
Change subject: soc/intel/common/block/sgx: Make PRMRR sizes always visible ......................................................................
soc/intel/common/block/sgx: Make PRMRR sizes always visible
Apparently, PRMRRs can be set to a non-zero size even if SGX is not to be enabled. This fixes boot failures when SGX is disabled.
Change-Id: I27f1d3741e8e3755130078c79ab13ae8873354fc Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/common/block/sgx/Kconfig 1 file changed, 18 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/45345/1
diff --git a/src/soc/intel/common/block/sgx/Kconfig b/src/soc/intel/common/block/sgx/Kconfig index 771c54c..b59c8ba 100644 --- a/src/soc/intel/common/block/sgx/Kconfig +++ b/src/soc/intel/common/block/sgx/Kconfig @@ -6,24 +6,6 @@ help Intel Processor common SGX support
-if SOC_INTEL_COMMON_BLOCK_SGX - -config SOC_INTEL_COMMON_BLOCK_SGX_LOCK_MEMORY - bool - default n - help - Lock memory before SGX activation. This is only needed if MCHECK does not do it. - -config SOC_INTEL_COMMON_BLOCK_SGX_ENABLE - bool "Enable Software Guard Extensions (SGX) if available" - default n - help - Intel Software Guard Extensions (SGX) is a set of new CPU instructions that can be - used by applications to set aside private regions (so-called Secure Enclaves) of - code and data. - - SGX will only be enabled when supported by the CPU! - config SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE int default 256 if SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE_MAX @@ -71,4 +53,22 @@
endchoice
+if SOC_INTEL_COMMON_BLOCK_SGX + +config SOC_INTEL_COMMON_BLOCK_SGX_LOCK_MEMORY + bool + default n + help + Lock memory before SGX activation. This is only needed if MCHECK does not do it. + +config SOC_INTEL_COMMON_BLOCK_SGX_ENABLE + bool "Enable Software Guard Extensions (SGX) if available" + default n + help + Intel Software Guard Extensions (SGX) is a set of new CPU instructions that can be + used by applications to set aside private regions (so-called Secure Enclaves) of + code and data. + + SGX will only be enabled when supported by the CPU! + endif
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45345 )
Change subject: soc/intel/common/block/sgx: Make PRMRR sizes always visible ......................................................................
Patch Set 1: Code-Review-1
Actually, PRMRR settings shouldn't be in SGX Kconfig at all.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45345 )
Change subject: soc/intel/common/block/sgx: Make PRMRR sizes always visible ......................................................................
Patch Set 1: Code-Review-1
Intel doc# 329298-002 makes more or less clear that PRMRR can only be used by SGX
Angel Pons has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/45345 )
Change subject: soc/intel/common/block/sgx: Make PRMRR sizes always visible ......................................................................
Abandoned
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45345 )
Change subject: soc/intel/common/block/sgx: Make PRMRR sizes always visible ......................................................................
Patch Set 1:
+2 best solution I've seen so far. We could move them into `common/block/cpu/Kconfig`? and s/_SGX//
Angel Pons has restored this change. ( https://review.coreboot.org/c/coreboot/+/45345 )
Change subject: soc/intel/common/block/sgx: Make PRMRR sizes always visible ......................................................................
Restored
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45345 )
Change subject: soc/intel/common/block/sgx: Make PRMRR sizes always visible ......................................................................
Patch Set 1: Code-Review+2
We could also move them into `common/block/cpu/Kconfig`? and `s/_SGX//`.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45345 )
Change subject: soc/intel/common/block/sgx: Make PRMRR sizes always visible ......................................................................
Patch Set 1:
Patch Set 1: Code-Review+2
We could also move them into `common/block/cpu/Kconfig`? and `s/_SGX//`.
I'm on it.
Hello build bot (Jenkins), Nico Huber, Michael Niewöhner, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45345
to look at the new patch set (#2).
Change subject: soc/intel/common/block: Move PRMRR options to CPU subfolder ......................................................................
soc/intel/common/block: Move PRMRR options to CPU subfolder
The PRMRR Kconfig symbols are located in the SGX subfolder. However, they are only used in `cpulib.c`, which lies within the CPU subfolder. Moreover, it seems that SGX isn't the only user of PRMRRs: there's an undocumented feature called C6DRAM, which apparently uses PRM as well.
Commit 1b89f5e (soc/intel/common/block/*/Kconfig: Guard options with if-blocks) made the PRMRR symbols only visible when the SoC selects SGX support with the `SOC_INTEL_COMMON_BLOCK_SGX` symbol. This resulted in `SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_DISABLED` not being selected anymore, which caused boot failures in `cpulib.c`, function `get_prmrr_size`.
Move all PRMRR size Kconfig symbols from SGX to CPU, and rename them. Also, do not check for `SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_DISABLED` to determine whether PRMRR is disabled. Instead, check if the configured PRMRR size is zero, and exit if so. Finally, add `default 0` lines to the `SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE` option, so that it always has a defined value, Prefer explicitness rather than less redundancy.
Change-Id: I27f1d3741e8e3755130078c79ab13ae8873354fc Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/sgx/Kconfig 3 files changed, 57 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/45345/2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45345 )
Change subject: soc/intel/common/block: Move PRMRR options to CPU subfolder ......................................................................
Patch Set 2: Code-Review+2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45345 )
Change subject: soc/intel/common/block: Move PRMRR options to CPU subfolder ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45345/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45345/2//COMMIT_MSG@12 PS2, Line 12: C6DRAM CFL FSP handles that case regardless of which PRMRR size is selected, other FSP might as well.
https://review.coreboot.org/c/coreboot/+/45345/2/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/45345/2/src/soc/intel/common/block/... PS2, Line 106: prompt "PRMRR size" With FSP doing all the magic for C6DRAM there's no need to show this when SGX isn't supported or disabled.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45345 )
Change subject: soc/intel/common/block: Move PRMRR options to CPU subfolder ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45345/2/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/45345/2/src/soc/intel/common/block/... PS2, Line 106: prompt "PRMRR size"
With FSP doing all the magic for C6DRAM there's no need to show this when SGX isn't supported or dis […]
Oh, I overlooked that...
898 /** Offset 0x0202 - C6DRAM power gating feature 899 This policy indicates whether or not BIOS should allocate PRMRR memory for C6DRAM 900 power gating feature.- 0: Don't allocate any PRMRR memory for C6DRAM power gating 901 feature.- <b>1: Allocate PRMRR memory for C6DRAM power gating feature</b>.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45345 )
Change subject: soc/intel/common/block: Move PRMRR options to CPU subfolder ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45345/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45345/2//COMMIT_MSG@12 PS2, Line 12: C6DRAM
CFL FSP handles that case regardless of which PRMRR size is selected, other FSP might as well.
skl doesn't support c6dram; cfl/cnl and later all set PRMRR when C6DRAM=1
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45345 )
Change subject: soc/intel/common/block: Move PRMRR options to CPU subfolder ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45345/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45345/2//COMMIT_MSG@12 PS2, Line 12: C6DRAM
skl doesn't support c6dram; cfl/cnl and later all set PRMRR when C6DRAM=1
well, but then it's only SGX in cb again... and we can leave it where it is lol
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45345 )
Change subject: soc/intel/common/block: Move PRMRR options to CPU subfolder ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45345/2/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/45345/2/src/soc/intel/common/block/... PS2, Line 106: prompt "PRMRR size"
Oh, I overlooked that... […]
Oooof, that changes everything, ofc. But that means we should also clarify a lot because we'd make the assumption that
The `PrmrrSize` UPD doesn't (fully) control the PRMRR size.
In that case, I vote for renaming get_prmrr_size() to make it clear that it's not the effective size. Currently it's only called to set the UPD. We should make sure nobody calls it and assumes that it will return the effective size.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45345 )
Change subject: soc/intel/common/block: Move PRMRR options to CPU subfolder ......................................................................
Patch Set 2: -Code-Review
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45345 )
Change subject: soc/intel/common/block: Move PRMRR options to CPU subfolder ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45345/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45345/2//COMMIT_MSG@12 PS2, Line 12: C6DRAM
well, but then it's only SGX in cb again... […]
If there's a C6DRAM enable switch that can be used in Kconfig, then we can use it here to hide PRMRR settings if C6DRAM is enabled.
https://review.coreboot.org/c/coreboot/+/45345/2/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/45345/2/src/soc/intel/common/block/... PS2, Line 106: prompt "PRMRR size"
Oh, I overlooked that... […]
If we don't, then the symbols are not defined, and the code in cpulib.c can go haywire (like it did). If FSP overrides PRMRR settings when C6DRAM is enabled, then we should hide this when C6DRAM is enabled.
Angel Pons has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/45345 )
Change subject: soc/intel/common/block: Move PRMRR options to CPU subfolder ......................................................................
Abandoned
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45345 )
Change subject: soc/intel/common/block: Move PRMRR options to CPU subfolder ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45345/2/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/45345/2/src/soc/intel/common/block/... PS2, Line 106: prompt "PRMRR size"
In that case, I vote for renaming get_prmrr_size() to make it clear that
it's not the effective size. Currently it's only called to set the UPD. We should make sure nobody calls it and assumes that it will return the effective size.
yep, maybe get_prmrr_setting?
If FSP overrides PRMRR settings when C6DRAM is enabled, then we should hide this when C6DRAM is enabled.
no, it's doesn't override it, but it sets it to 1M, when EnableC6Dram=1 && PrmrrSize=0
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45345 )
Change subject: soc/intel/common/block: Move PRMRR options to CPU subfolder ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45345/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45345/2//COMMIT_MSG@12 PS2, Line 12: C6DRAM no, there is no Kconfig and it would be wrong anyways; see https://review.coreboot.org/c/coreboot/+/45345/2/src/soc/intel/common/block/...
If FSP overrides PRMRR settings when C6DRAM is enabled, then we should hide this when C6DRAM is enabled.
no, it's doesn't override it, but it sets it to 1M, when EnableC6Dram=1 && PrmrrSize=0
Angel Pons has restored this change. ( https://review.coreboot.org/c/coreboot/+/45345 )
Change subject: soc/intel/common/block: Move PRMRR options to CPU subfolder ......................................................................
Restored
Angel Pons has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/45345 )
Change subject: soc/intel/common/block: Move PRMRR options to CPU subfolder ......................................................................
Abandoned
This would be correct if FSP didn't exist. However, it seems to take care of PRMRRs in the C6DRAM case already.
-
Angel Pons (Code Review) -
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)