Michael Niewöhner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: make SGX a Kconfig option ......................................................................
soc/intel/sgx: make SGX a Kconfig option
This removes the devicetree parameter sgx_enable and add a Kconfig option instead, as the devictree is not made for user-choosable options.
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb M src/mainboard/supermicro/x11-lga1151-series/devicetree.cb M src/soc/intel/apollolake/chip.h M src/soc/intel/apollolake/cpu.c M src/soc/intel/apollolake/memmap.c M src/soc/intel/common/block/include/intelblocks/sgx.h M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c 10 files changed, 19 insertions(+), 73 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/1
diff --git a/src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb b/src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb index d3d0b00..9277f08 100644 --- a/src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb +++ b/src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb @@ -97,8 +97,6 @@ # Minimum SLP S3 assertion width 28ms. register "slp_s3_assertion_width_usecs" = "28000"
- register "sgx_enable" = "1" - # PRMRR size options # 0x02000000 - 32MiB # 0x04000000 - 64MiB diff --git a/src/mainboard/supermicro/x11-lga1151-series/devicetree.cb b/src/mainboard/supermicro/x11-lga1151-series/devicetree.cb index a5ff0c5..0c42edd 100644 --- a/src/mainboard/supermicro/x11-lga1151-series/devicetree.cb +++ b/src/mainboard/supermicro/x11-lga1151-series/devicetree.cb @@ -17,8 +17,6 @@ register "Device4Enable" = "1" register "SaGv" = "SaGv_Disabled"
- # Disable SGX - register "sgx_enable" = "0" # SGX is broken in coreboot register "PrmrrSize" = "128 * MiB"
register "pirqa_routing" = "PCH_IRQ11" diff --git a/src/soc/intel/apollolake/chip.h b/src/soc/intel/apollolake/chip.h index 85cfff9..3fdc646 100644 --- a/src/soc/intel/apollolake/chip.h +++ b/src/soc/intel/apollolake/chip.h @@ -145,12 +145,6 @@ * 0x08000000 - 128MiB */ uint32_t PrmrrSize;
- /* Enable SGX feature. - * Enabling SGX feature is 2 step process, - * (1) set sgx_enable = 1 - * (2) set PrmrrSize to supported size */ - uint8_t sgx_enable; - /* Select PNP Settings. * (0) Performance, * (1) Power diff --git a/src/soc/intel/apollolake/cpu.c b/src/soc/intel/apollolake/cpu.c index 3349627..4ec714f 100644 --- a/src/soc/intel/apollolake/cpu.c +++ b/src/soc/intel/apollolake/cpu.c @@ -293,11 +293,3 @@ /* Do nothing because MCHECK while loading microcode and enabling * IA untrusted mode takes care of necessary locking */ } - -int soc_fill_sgx_param(struct sgx_param *sgx_param) -{ - config_t *conf = config_of_soc(); - - sgx_param->enable = conf->sgx_enable; - return 0; -} diff --git a/src/soc/intel/apollolake/memmap.c b/src/soc/intel/apollolake/memmap.c index 77711eb..5cdd43b 100644 --- a/src/soc/intel/apollolake/memmap.c +++ b/src/soc/intel/apollolake/memmap.c @@ -35,11 +35,11 @@ if (!CONFIG(SOC_INTEL_GLK)) return tolum;
- config = config_of_soc(); - /* FSP allocates 2x PRMRR Size Memory for alignment */ - if (config->sgx_enable) + if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE)) { + config = config_of_soc(); tolum -= config->PrmrrSize * 2; + }
return tolum; } diff --git a/src/soc/intel/common/block/include/intelblocks/sgx.h b/src/soc/intel/common/block/include/intelblocks/sgx.h index 502483f..693dd20 100644 --- a/src/soc/intel/common/block/include/intelblocks/sgx.h +++ b/src/soc/intel/common/block/include/intelblocks/sgx.h @@ -18,10 +18,6 @@
#include <soc/nvs.h>
-struct sgx_param { - uint8_t enable; -}; - /* * Lock SGX memory. * CPU specific code needs to provide the implementation. @@ -40,10 +36,6 @@ */ void sgx_configure(void *unused);
-/* SOC specific API to get SGX params. - * returns 0, if able to get SGX params; otherwise returns -1 */ -int soc_fill_sgx_param(struct sgx_param *sgx_param); - /* Fill GNVS data with SGX status, EPC base and length */ void sgx_fill_gnvs(global_nvs_t *gnvs);
diff --git a/src/soc/intel/common/block/sgx/Kconfig b/src/soc/intel/common/block/sgx/Kconfig index 0852bfb..78284fb 100644 --- a/src/soc/intel/common/block/sgx/Kconfig +++ b/src/soc/intel/common/block/sgx/Kconfig @@ -2,6 +2,12 @@ bool default n help - Software Guard eXtension(SGX) Feature. Intel SGX is a set of new CPU - instructions that can be used by applications to set aside private - regions of code and data. + Intel Processor common SGX support + +config SOC_INTEL_COMMON_BLOCK_SGX_ENABLE + bool + depends on SOC_INTEL_COMMON_BLOCK_SGX + default n + help + Software Guard eXtension (SGX) Feature. Intel SGX is a set of new CPU instructions + that can be used by applications to set aside private regions of code and data. diff --git a/src/soc/intel/common/block/sgx/sgx.c b/src/soc/intel/common/block/sgx/sgx.c index 60714d9..4567e13 100644 --- a/src/soc/intel/common/block/sgx/sgx.c +++ b/src/soc/intel/common/block/sgx/sgx.c @@ -25,9 +25,6 @@ #include <soc/pci_devs.h> #include <string.h>
-static bool sgx_param_valid; -static struct sgx_param g_sgx_param; - static inline uint64_t sgx_resource(uint32_t low, uint32_t high) { uint64_t val; @@ -36,28 +33,6 @@ return val; }
-static const struct sgx_param *get_sgx_param(void) -{ - if (sgx_param_valid) - return &g_sgx_param; - - memset(&g_sgx_param, 0, sizeof(g_sgx_param)); - if (soc_fill_sgx_param(&g_sgx_param) < 0) { - printk(BIOS_ERR, "SGX : Failed to get soc sgx param\n"); - return NULL; - } - sgx_param_valid = true; - printk(BIOS_INFO, "SGX : param.enable = %d\n", g_sgx_param.enable); - - return &g_sgx_param; -} - -static int soc_sgx_enabled(void) -{ - const struct sgx_param *sgx_param = get_sgx_param(); - return sgx_param ? sgx_param->enable : 0; -} - static int is_sgx_supported(void) { struct cpuid_result cpuid_regs; @@ -79,7 +54,7 @@ } prmrr_base, prmrr_mask; msr_t msr;
- if (!soc_sgx_enabled() || !is_sgx_supported()) + if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE) || !is_sgx_supported()) return;
msr = rdmsr(MSR_PRMRR_PHYS_MASK); @@ -204,7 +179,7 @@ { const void *microcode_patch = intel_mp_current_microcode();
- if (!soc_sgx_enabled() || !is_sgx_supported() || !is_prmrr_set()) { + if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX) || !is_sgx_supported() || !is_prmrr_set()) { printk(BIOS_ERR, "SGX: pre-conditions not met\n"); return; } @@ -234,7 +209,7 @@ { struct cpuid_result cpuid_regs;
- if (!soc_sgx_enabled() || !is_sgx_supported()) { + if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX) || !is_sgx_supported()) { printk(BIOS_DEBUG, "SGX: not enabled or not supported. skip gnvs fill\n"); return; diff --git a/src/soc/intel/skylake/chip.h b/src/soc/intel/skylake/chip.h index 944315b..626d36b 100644 --- a/src/soc/intel/skylake/chip.h +++ b/src/soc/intel/skylake/chip.h @@ -576,9 +576,6 @@ u8 SlowSlewRateForGt; u8 SlowSlewRateForSa;
- /* Enable SGX feature */ - u8 sgx_enable; - /* Enable/Disable EIST * 1b - Enabled * 0b - Disabled diff --git a/src/soc/intel/skylake/cpu.c b/src/soc/intel/skylake/cpu.c index 5424c91..26cc533 100644 --- a/src/soc/intel/skylake/cpu.c +++ b/src/soc/intel/skylake/cpu.c @@ -455,7 +455,8 @@ enable_turbo();
/* Configure Core PRMRR for SGX. */ - prmrr_core_configure(); + if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX)) + prmrr_core_configure(); }
static void per_cpu_smm_trigger(void) @@ -493,7 +494,8 @@
ret |= mp_run_on_all_cpus(vmx_configure, NULL);
- ret |= mp_run_on_all_cpus(sgx_configure, NULL); + if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX)) + ret |= mp_run_on_all_cpus(sgx_configure, NULL);
ret |= mp_run_on_all_cpus(fc_lock_configure, NULL);
@@ -559,11 +561,3 @@ wrmsr(MSR_LT_LOCK_MEMORY, msr); } } - -int soc_fill_sgx_param(struct sgx_param *sgx_param) -{ - config_t *conf = config_of_soc(); - - sgx_param->enable = conf->sgx_enable; - return 0; -}
Hello Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#2).
Change subject: soc/intel/sgx: make SGX a Kconfig option ......................................................................
soc/intel/sgx: make SGX a Kconfig option
This removes the devicetree parameter sgx_enable and add a Kconfig option instead, as the devictree is not made for user-choosable options.
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb M src/mainboard/supermicro/x11-lga1151-series/devicetree.cb M src/soc/intel/apollolake/chip.h M src/soc/intel/apollolake/cpu.c M src/soc/intel/apollolake/memmap.c M src/soc/intel/common/block/include/intelblocks/sgx.h M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c 10 files changed, 19 insertions(+), 73 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: make SGX a Kconfig option ......................................................................
Patch Set 2: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/35799/2/src/mainboard/intel/glkrvp/... File src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35799/2/src/mainboard/intel/glkrvp/... PS2, Line 100: register "sgx_enable" = "1" add select SOC_INTEL_COMMON_BLOCK_SGX_ENABLE to keep existing behaviour
https://review.coreboot.org/c/coreboot/+/35799/2/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/Kconfig:
https://review.coreboot.org/c/coreboot/+/35799/2/src/soc/intel/common/block/... PS2, Line 10: n default y?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: make SGX a Kconfig option ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/2/src/soc/intel/apollolake/me... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/2/src/soc/intel/apollolake/me... PS2, Line 41: PrmrrSize I find it weird that this is an FSP-M parameter, but sgx_enable isn't. So the prmrrSize region is set up by FSP-M but is just ram unless sgx is enabled?
I have no knowledge on either but this deserves an explanation in comments.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: make SGX a Kconfig option ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/2/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/Kconfig:
https://review.coreboot.org/c/coreboot/+/35799/2/src/soc/intel/common/block/... PS2, Line 10: n
default y?
only x11* and glkrvp had it enabled, before. I guess not keeping exisiting defaults for two boards is better than changing it for all others
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: make SGX a Kconfig option ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/2/src/soc/intel/apollolake/me... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/2/src/soc/intel/apollolake/me... PS2, Line 41: PrmrrSize
I find it weird that this is an FSP-M parameter, but sgx_enable isn't. […]
Correct,it is "just ram" but as soon as PRMRR gets enabled and locked, any access to it will be simply dropped by the CPU. Coreboot enables PRMRR only, when SGX is enabled. I'll add a comment on this.
I don't know what FSP-M really does but it does not lock/enable the range because we wouldn't be able to set and enable it after locking. I guess it just needs it for calculation of other ranges.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: make SGX a Kconfig option ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/2/src/soc/intel/apollolake/me... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/2/src/soc/intel/apollolake/me... PS2, Line 41: PrmrrSize
Correct,it is "just ram" but as soon as PRMRR gets enabled and locked, any access to it will be simply dropped by the CPU. Coreboot enables PRMRR only, when SGX is enabled. I'll add a comment on this.
I don't know what FSP-M really does but it does not lock/enable the range because we wouldn't be able to set and enable it after locking. I guess it just needs it for calculation of other ranges.
It's not like this change is introducing a regression, but wouldn't you also want this to depend on whether the system actually supports SGX?
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#3).
Change subject: soc/intel/sgx: make SGX a Kconfig option ......................................................................
soc/intel/sgx: make SGX a Kconfig option
This removes the devicetree parameter sgx_enable and add a Kconfig option instead, as the devictree is not made for user-choosable options.
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/intel/glkrvp/Kconfig M src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb M src/mainboard/supermicro/x11-lga1151-series/devicetree.cb M src/soc/intel/apollolake/chip.h M src/soc/intel/apollolake/cpu.c M src/soc/intel/apollolake/memmap.c M src/soc/intel/common/block/include/intelblocks/sgx.h M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c 11 files changed, 32 insertions(+), 77 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/3
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: make SGX a Kconfig option ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/2/src/mainboard/intel/glkrvp/... File src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35799/2/src/mainboard/intel/glkrvp/... PS2, Line 100: register "sgx_enable" = "1"
add select SOC_INTEL_COMMON_BLOCK_SGX_ENABLE to keep existing behaviour
Done
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: make SGX a Kconfig option ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/3/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/sgx.c:
https://review.coreboot.org/c/coreboot/+/35799/3/src/soc/intel/common/block/... PS3, Line 182: if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE) || !is_sgx_supported() || suspect code indent for conditional statements (8, 24)
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: make SGX a Kconfig option ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/2/src/soc/intel/apollolake/me... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/2/src/soc/intel/apollolake/me... PS2, Line 41: PrmrrSize
Correct,it is "just ram" but as soon as PRMRR gets enabled and locked, any access to it will be si […]
We need to figure out: - is prmrr needed for anything else? (browell configures it, but does not have sgx) - when not enabling prmrr, do we need to adapt the memory layout anyways, e.g. because fsp expects it? or should we configure it to the lowest value (1MB)? - should we simply detect and set the highest possible prmrr or let the user choose and die() if the selected value is unsupported?
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#4).
Change subject: soc/intel/sgx: make SGX a Kconfig option ......................................................................
soc/intel/sgx: make SGX a Kconfig option
This removes the devicetree parameter sgx_enable and add a Kconfig option instead, as the devictree is not made for user-choosable options.
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/intel/glkrvp/Kconfig M src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb M src/mainboard/supermicro/x11-lga1151-series/devicetree.cb M src/soc/intel/apollolake/chip.h M src/soc/intel/apollolake/cpu.c M src/soc/intel/apollolake/memmap.c M src/soc/intel/common/block/include/intelblocks/sgx.h M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c 11 files changed, 34 insertions(+), 77 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: make SGX a Kconfig option ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/4/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/sgx.c:
https://review.coreboot.org/c/coreboot/+/35799/4/src/soc/intel/common/block/... PS4, Line 182: if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE) || !is_sgx_supported() || suspect code indent for conditional statements (8, 24)
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 5:
This change is ready for review.
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#6).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is nod made for user-choosable options, thus introduce a Kconfig option for both SGX and the corresponding PRMRR size.
PRMRR size can only be set when SGX is enabled. This is because currently that is the safest option for the user, since we do not really know when the FSP configures PRMRR to which size depending on SGX status. Hence, provide the highest possible compatibility to the prior devicetree options.
As soon as we know more about PRMRR in general and when to set which size, the PRMRR Kconfig can be extendend.
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/include/cpu/x86/mtrr.h M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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 M src/soc/intel/apollolake/cpu.c 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/common/block/include/intelblocks/sgx.h M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c 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/cpu.c M src/soc/intel/skylake/memmap.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 27 files changed, 193 insertions(+), 169 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/6
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 336: for (i=8; i >= 0; i--)
spaces required around that '=' (ctx:VxV)
Done
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 337: if (valid = msr.lo & (1<<i))
do not use assignment in if condition
Done
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/sgx.c:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 182: if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE) || !is_sgx_supported() ||
suspect code indent for conditional statements (8, 24)
????
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/6/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/sgx.c:
https://review.coreboot.org/c/coreboot/+/35799/6/src/soc/intel/common/block/... PS6, Line 182: if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE) || !is_sgx_supported() || suspect code indent for conditional statements (8, 24)
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#7).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is nod made for user-choosable options, thus introduce a Kconfig option for both SGX and the corresponding PRMRR size.
PRMRR size can only be set when SGX is enabled. This is because currently that is the safest option for the user, since we do not really know when the FSP configures PRMRR to which size depending on SGX status. Hence, provide the highest possible compatibility to the prior devicetree options.
As soon as we know more about PRMRR in general and when to set which size, the PRMRR Kconfig can be extendend.
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/include/cpu/x86/mtrr.h M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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 M src/soc/intel/apollolake/cpu.c 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/common/block/include/intelblocks/sgx.h M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c 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/cpu.c M src/soc/intel/skylake/memmap.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 27 files changed, 200 insertions(+), 169 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/7/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/sgx.c:
https://review.coreboot.org/c/coreboot/+/35799/7/src/soc/intel/common/block/... PS7, Line 182: if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE) || !is_sgx_supported() || suspect code indent for conditional statements (8, 24)
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 5:
(12 comments)
https://review.coreboot.org/c/coreboot/+/35799/5/src/mainboard/intel/glkrvp/... File src/mainboard/intel/glkrvp/Kconfig:
https://review.coreboot.org/c/coreboot/+/35799/5/src/mainboard/intel/glkrvp/... PS5, Line 89: config SOC_INTEL_COMMON_BLOCK_SGX_ENABLE : bool : default y Do you want to move the devicetree setting (supposedly mb specific) to a SoC default?
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/apollolake/me... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/apollolake/me... PS5, Line 35: CONFIG( not a Kconfig
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 332: valid uninitialized?
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 340: valid/MiB) Will always be 0.
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/sgx.h:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 45: soc_fill_sgx_param The apollolake definition still exists.
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/Kconfig:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 31: SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE_256 Always be specific about units -> 256*MB* ;)
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/sgx.c:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 39: static const struct sgx_param *get_sgx_param(void) : { : if (sgx_param_valid) : return &g_sgx_param; : : memset(&g_sgx_param, 0, sizeof(g_sgx_param)); : if (soc_fill_sgx_param(&g_sgx_param) < 0) { : printk(BIOS_ERR, "SGX : Failed to get soc sgx param\n"); : return NULL; : } : sgx_param_valid = true; : printk(BIOS_INFO, "SGX : param.enable = %d\n", g_sgx_param.enable); : : return &g_sgx_param; : } : : static int soc_sgx_enabled(void) : { : const struct sgx_param *sgx_param = get_sgx_param(); : return sgx_param ? sgx_param->enable : 0; : } It might be easier to get rid of this crap in a separate patch. I'd +2 that easily ;)
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 182: !CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE) Move this up and just return. It's noise in the log to tell the user SGX conditions are not set if it's disabled in Kconfig
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 213: CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE) same
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/icelake/chip.... File src/soc/intel/icelake/chip.h:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/icelake/chip.... PS5, Line 278: get_prmrr_size Move the function declaration to a common place?
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/skylake/cpu.c File src/soc/intel/skylake/cpu.c:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/skylake/cpu.c... PS5, Line 498: sgx_configure You are checking for the Kconfig option in the call too
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/skylake/memma... File src/soc/intel/skylake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/skylake/memma... PS5, Line 89: no empty line
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35799/7/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/sgx.c:
https://review.coreboot.org/c/coreboot/+/35799/7/src/soc/intel/common/block/... PS7, Line 28: static bool sgx_param_valid; the file sgx.c needs to be compiled if SOC_INTEL_COMMON_BLOCK_SGX_ENABLE is y
https://review.coreboot.org/c/coreboot/+/35799/7/src/soc/intel/common/block/... PS7, Line 57: if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE) || !is_sgx_supported()) that check should be in the function calling prmrr_core_configure()
https://review.coreboot.org/c/coreboot/+/35799/7/src/soc/intel/common/block/... PS7, Line 182: if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE) || !is_sgx_supported() || that check should be in the code calling sgx_configure()
https://review.coreboot.org/c/coreboot/+/35799/7/src/soc/intel/common/block/... PS7, Line 213: if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE) || !is_sgx_supported()) { should be in the code calling sgx_fill_gnvs()
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 7:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 332: valid
uninitialized?
already fixed in newest patchset
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 340: valid/MiB)
Will always be 0.
no, but I changed this anyways in the newst patchset :-)
https://review.coreboot.org/c/coreboot/+/35799/7/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/sgx.c:
https://review.coreboot.org/c/coreboot/+/35799/7/src/soc/intel/common/block/... PS7, Line 28: static bool sgx_param_valid;
the file sgx. […]
?? this is the case already as SOC_INTEL_COMMON_BLOCK_SGX_ENABLE depends on SOC_INTEL_COMMON_BLOCK_SGX; see Kconfig and Makefile.inc
https://review.coreboot.org/c/coreboot/+/35799/7/src/soc/intel/common/block/... PS7, Line 57: if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE) || !is_sgx_supported())
that check should be in the function calling prmrr_core_configure()
why? this would change the behaviour which I strictly want to avoid
https://review.coreboot.org/c/coreboot/+/35799/7/src/soc/intel/common/block/... PS7, Line 213: if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE) || !is_sgx_supported()) {
should be in the code calling sgx_fill_gnvs()
same as above
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 7:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35799/5/src/mainboard/intel/glkrvp/... File src/mainboard/intel/glkrvp/Kconfig:
https://review.coreboot.org/c/coreboot/+/35799/5/src/mainboard/intel/glkrvp/... PS5, Line 89: config SOC_INTEL_COMMON_BLOCK_SGX_ENABLE : bool : default y
Do you want to move the devicetree setting (supposedly mb specific) to a SoC default?
Sorry, I don't get what you mean. sgx was enabled in the devicetree (yes, mb specific) before. now it gets enabled via the new Kconfig setting in the boards Kconfig (the mb specific one). I don't want to change the default behaviour as I don't know if SGX is working on all other boards.
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/sgx.h:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 45: soc_fill_sgx_param
The apollolake definition still exists.
huh? apollolake gets removed https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/apollolake/cp...
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/icelake/chip.... File src/soc/intel/icelake/chip.h:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/icelake/chip.... PS5, Line 278: get_prmrr_size
Move the function declaration to a common place?
as soon is we know more about prmrr, I want to move the whole prmrr stuff to the common section. atm I would leave it here
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/skylake/cpu.c File src/soc/intel/skylake/cpu.c:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/skylake/cpu.c... PS5, Line 498: sgx_configure
You are checking for the Kconfig option in the call too
yeah, I'm not sure yet, what is better. check it here or check it in sgx_configure?
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/skylake/memma... File src/soc/intel/skylake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/skylake/memma... PS5, Line 89:
no empty line
readability? can remove it if you insist
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35799/2/src/soc/intel/apollolake/me... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/2/src/soc/intel/apollolake/me... PS2, Line 41: PrmrrSize
We need to figure out: […]
Done
https://review.coreboot.org/c/coreboot/+/35799/2/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/Kconfig:
https://review.coreboot.org/c/coreboot/+/35799/2/src/soc/intel/common/block/... PS2, Line 10: n
only x11* and glkrvp had it enabled, before. […]
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35799/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35799/7//COMMIT_MSG@9 PS7, Line 9: nod not
https://review.coreboot.org/c/coreboot/+/35799/7//COMMIT_MSG@19 PS7, Line 19: extendend extended
https://review.coreboot.org/c/coreboot/+/35799/7/src/soc/intel/apollolake/me... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/7/src/soc/intel/apollolake/me... PS7, Line 53: */ Please use one of the recommended multi-line comment styles.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 7:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/apollolake/me... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/apollolake/me... PS5, Line 35: CONFIG(
not a Kconfig
Done
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/sgx.h:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 45: soc_fill_sgx_param
huh? apollolake gets removed https://review.coreboot. […]
Done
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/Kconfig:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 31: SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE_256
Always be specific about units -> 256*MB* ;)
Done
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/sgx.c:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 39: static const struct sgx_param *get_sgx_param(void) : { : if (sgx_param_valid) : return &g_sgx_param; : : memset(&g_sgx_param, 0, sizeof(g_sgx_param)); : if (soc_fill_sgx_param(&g_sgx_param) < 0) { : printk(BIOS_ERR, "SGX : Failed to get soc sgx param\n"); : return NULL; : } : sgx_param_valid = true; : printk(BIOS_INFO, "SGX : param.enable = %d\n", g_sgx_param.enable); : : return &g_sgx_param; : } : : static int soc_sgx_enabled(void) : { : const struct sgx_param *sgx_param = get_sgx_param(); : return sgx_param ? sgx_param->enable : 0; : }
It might be easier to get rid of this crap in a separate patch. […]
Done
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 182: !CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE)
Move this up and just return. […]
check now on invocation instead
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 213: CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE)
same
check now on invocation instead
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/skylake/cpu.c File src/soc/intel/skylake/cpu.c:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/skylake/cpu.c... PS5, Line 498: sgx_configure
yeah, I'm not sure yet, what is better. […]
check now on invocation only
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#8).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is nod made for user-choosable options, thus introduce a Kconfig option for both SGX and the corresponding PRMRR size.
PRMRR size can only be set when SGX is enabled. This is because currently that is the safest option for the user, since we do not really know when the FSP configures PRMRR to which size depending on SGX status. Hence, provide the highest possible compatibility to the prior devicetree options.
As soon as we know more about PRMRR in general and when to set which size, the PRMRR Kconfig can be extendend.
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/include/cpu/x86/mtrr.h M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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 M src/soc/intel/apollolake/cpu.c 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/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c 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/cpu.c M src/soc/intel/skylake/memmap.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 26 files changed, 195 insertions(+), 118 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/8
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/8/src/soc/intel/apollolake/cp... File src/soc/intel/apollolake/cpu.c:
https://review.coreboot.org/c/coreboot/+/35799/8/src/soc/intel/apollolake/cp... PS8, Line 80: if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE)) || trailing statements should be on next line
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#9).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce a Kconfig option for both SGX and the corresponding PRMRR size.
PRMRR size can only be set when SGX is enabled. This is because currently that is the safest option for the user, since we do not really know when the FSP configures PRMRR to which size depending on SGX status. Hence, provide the highest possible compatibility to the prior devicetree options.
As soon as we know more about PRMRR in general and when to set which size, the PRMRR Kconfig can be extended.
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/include/cpu/x86/mtrr.h M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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 M src/soc/intel/apollolake/cpu.c 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/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c 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/cpu.c M src/soc/intel/skylake/memmap.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 26 files changed, 195 insertions(+), 118 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/9
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 9:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35799/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35799/7//COMMIT_MSG@9 PS7, Line 9: nod
not
Done
https://review.coreboot.org/c/coreboot/+/35799/7//COMMIT_MSG@19 PS7, Line 19: extendend
extended
Done
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/sgx.c:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 39: static const struct sgx_param *get_sgx_param(void) : { : if (sgx_param_valid) : return &g_sgx_param; : : memset(&g_sgx_param, 0, sizeof(g_sgx_param)); : if (soc_fill_sgx_param(&g_sgx_param) < 0) { : printk(BIOS_ERR, "SGX : Failed to get soc sgx param\n"); : return NULL; : } : sgx_param_valid = true; : printk(BIOS_INFO, "SGX : param.enable = %d\n", g_sgx_param.enable); : : return &g_sgx_param; : } : : static int soc_sgx_enabled(void) : { : const struct sgx_param *sgx_param = get_sgx_param(); : return sgx_param ? sgx_param->enable : 0; : }
Done
https://review.coreboot.org/c/coreboot/+/35882
https://review.coreboot.org/c/coreboot/+/35799/7/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/sgx.c:
https://review.coreboot.org/c/coreboot/+/35799/7/src/soc/intel/common/block/... PS7, Line 57: if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE) || !is_sgx_supported())
why? this would change the behaviour which I strictly want to avoid
just forget what I wrote...
https://review.coreboot.org/c/coreboot/+/35799/7/src/soc/intel/common/block/... PS7, Line 213: if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE) || !is_sgx_supported()) {
same as above
Done
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#10).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce a Kconfig option for both SGX and the corresponding PRMRR size.
PRMRR size can only be set when SGX is enabled. This is because currently that is the safest option for the user, since we do not really know when the FSP configures PRMRR to which size depending on SGX status. Hence, provide the highest possible compatibility to the prior devicetree options.
As soon as we know more about PRMRR in general and when to set which size, the PRMRR Kconfig can be extended.
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/include/cpu/x86/mtrr.h M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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 M src/soc/intel/apollolake/cpu.c 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/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c 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/cpu.c M src/soc/intel/skylake/memmap.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 26 files changed, 196 insertions(+), 118 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/10
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/7/src/soc/intel/apollolake/me... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/7/src/soc/intel/apollolake/me... PS7, Line 53: */
Please use one of the recommended multi-line comment styles.
Done
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/10/src/soc/intel/apollolake/c... File src/soc/intel/apollolake/cpu.c:
https://review.coreboot.org/c/coreboot/+/35799/10/src/soc/intel/apollolake/c... PS10, Line 80: if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE)) || trailing statements should be on next line
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#11).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce a Kconfig option for both SGX and the corresponding PRMRR size.
PRMRR size can only be set when SGX is enabled. This is because currently that is the safest option for the user, since we do not really know when the FSP configures PRMRR to which size depending on SGX status. Hence, provide the highest possible compatibility to the prior devicetree options.
As soon as we know more about PRMRR in general and when to set which size, the PRMRR Kconfig can be extended.
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/include/cpu/x86/mtrr.h M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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 M src/soc/intel/apollolake/cpu.c 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/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c 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/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/memmap.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 27 files changed, 197 insertions(+), 119 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/11
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/11/src/soc/intel/apollolake/c... File src/soc/intel/apollolake/cpu.c:
https://review.coreboot.org/c/coreboot/+/35799/11/src/soc/intel/apollolake/c... PS11, Line 80: if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE)) || trailing statements should be on next line
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#12).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce a Kconfig option for both SGX and the corresponding PRMRR size.
PRMRR size can only be set when SGX is enabled. This is because currently that is the safest option for the user, since we do not really know when the FSP configures PRMRR to which size depending on SGX status. Hence, provide the highest possible compatibility to the prior devicetree options.
As soon as we know more about PRMRR in general and when to set which size, the PRMRR Kconfig can be extended.
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/include/cpu/x86/mtrr.h M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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 M src/soc/intel/apollolake/cpu.c 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/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c 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/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/memmap.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 27 files changed, 197 insertions(+), 120 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/12
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35799/10/src/soc/intel/apollolake/c... File src/soc/intel/apollolake/cpu.c:
https://review.coreboot.org/c/coreboot/+/35799/10/src/soc/intel/apollolake/c... PS10, Line 80: if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE)) ||
trailing statements should be on next line
Done
https://review.coreboot.org/c/coreboot/+/35799/11/src/soc/intel/apollolake/c... File src/soc/intel/apollolake/cpu.c:
https://review.coreboot.org/c/coreboot/+/35799/11/src/soc/intel/apollolake/c... PS11, Line 80: if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE)) ||
trailing statements should be on next line
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 12:
Sorry for jumping into a review late. Is it too late to split this up? The `memmap.c` changes are an actual fix to the code base, are the fragile part that needs decent review _and_ are not covered by the commit message. All clear signs that they belong into a preceding commit.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 12:
Patch Set 12:
Sorry for jumping into a review late. Is it too late to split this up? The `memmap.c` changes are an actual fix to the code base, are the fragile part that needs decent review _and_ are not covered by the commit message. All clear signs that they belong into a preceding commit.
Well, let's see if we get this splitted :-)
not covered by the commit message
They ARE covered (second parapraph), just not that deeply. I already splitted of, what Arthur suggested. What do you want to see moved to the previous commit? The remaining memmap changes are needed to make it configurable.
I could move the msr checks to another commit if you like.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35799/12/src/soc/intel/icelake/chip... File src/soc/intel/icelake/chip.h:
https://review.coreboot.org/c/coreboot/+/35799/12/src/soc/intel/icelake/chip... PS12, Line 278: get_prmrr_size This is not a good place for function declaration
https://review.coreboot.org/c/coreboot/+/35799/12/src/soc/intel/icelake/memm... File src/soc/intel/icelake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/12/src/soc/intel/icelake/memm... PS12, Line 51: get_prmrr_size I'd use a common definition. You can get the chip config in the implementation instead of via the argument.
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#13).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
PRMRR size can only be set when SGX is enabled. Currently, that is the safest option for the user, since we do not really know when the FSP configures PRMRR to which size depending on SGX status. Hence, use the previously implemented algorithm to find a valid PRMRR size with respect to the user-specified maximum.
As soon as we know more about PRMRR in general and when to set which size, the PRMRR Kconfig can be extended.
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/include/cpu/x86/mtrr.h M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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 M src/soc/intel/apollolake/cpu.c M src/soc/intel/apollolake/memmap.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/memmap.c M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/memmap.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/memmap.c 20 files changed, 72 insertions(+), 68 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/13
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 13:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/sgx.c:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 213: CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE)
check now on invocation instead
Done
https://review.coreboot.org/c/coreboot/+/35799/12/src/soc/intel/icelake/chip... File src/soc/intel/icelake/chip.h:
https://review.coreboot.org/c/coreboot/+/35799/12/src/soc/intel/icelake/chip... PS12, Line 278: get_prmrr_size
This is not a good place for function declaration
If you know a better place, tell me, please. I wasn't sure where this is put best :)
https://review.coreboot.org/c/coreboot/+/35799/12/src/soc/intel/icelake/memm... File src/soc/intel/icelake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/12/src/soc/intel/icelake/memm... PS12, Line 51: get_prmrr_size
I'd use a common definition. […]
Done
Hello Patrick Rudolph, 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/+/35799
to look at the new patch set (#14).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
PRMRR size can only be set when SGX is enabled. Currently, that is the safest option for the user, since we do not really know when the FSP configures PRMRR to which size depending on SGX status. Hence, use the previously implemented algorithm to find a valid PRMRR size with respect to the user-specified maximum.
As soon as we know more about PRMRR in general and when to set which size, the PRMRR Kconfig can be extended.
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/include/cpu/x86/mtrr.h M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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 M src/soc/intel/apollolake/cpu.c M src/soc/intel/apollolake/memmap.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/memmap.c M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/memmap.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/memmap.c 20 files changed, 72 insertions(+), 68 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/14
Hello Patrick Rudolph, 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/+/35799
to look at the new patch set (#17).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
PRMRR size can only be set when SGX is enabled. Currently, that is the safest option for the user, since we do not really know when the FSP configures PRMRR to which size depending on SGX status. Hence, use the previously implemented algorithm to find a valid PRMRR size with respect to the user-specified maximum.
As soon as we know more about PRMRR in general and when to set which size, the PRMRR Kconfig can be extended.
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/include/cpu/x86/mtrr.h M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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 M src/soc/intel/apollolake/cpu.c M src/soc/intel/apollolake/memmap.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/memmap.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/memmap.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/memmap.c 21 files changed, 73 insertions(+), 73 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/17
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 18: Patch Set 17 was rebased.
Hello Patrick Rudolph, 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/+/35799
to look at the new patch set (#19).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
PRMRR size can only be set when SGX is enabled. Currently, that is the safest option for the user, since we do not really know when the FSP configures PRMRR to which size depending on SGX status. Hence, use the previously implemented algorithm to find a valid PRMRR size with respect to the user-specified maximum.
As soon as we know more about PRMRR in general and when to set which size, the PRMRR Kconfig can be extended.
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/include/cpu/x86/mtrr.h M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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 M src/soc/intel/apollolake/cpu.c M src/soc/intel/apollolake/memmap.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/memmap.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/memmap.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/memmap.c 21 files changed, 73 insertions(+), 73 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/19
Hello Patrick Rudolph, 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/+/35799
to look at the new patch set (#20).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
PRMRR size can only be set when SGX is enabled. Currently, that is the safest option for the user, since we do not really know when the FSP configures PRMRR to which size depending on SGX status. Hence, use the previously implemented algorithm to find a valid PRMRR size with respect to the user-specified maximum.
As soon as we know more about PRMRR in general and when to set which size, the PRMRR Kconfig can be extended.
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/include/cpu/x86/mtrr.h M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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 M src/soc/intel/apollolake/cpu.c M src/soc/intel/apollolake/memmap.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/memmap.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/memmap.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/memmap.c 21 files changed, 73 insertions(+), 76 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/20
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 20.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 21.
Hello Patrick Rudolph, 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/+/35799
to look at the new patch set (#21).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
PRMRR size can only be set when SGX is enabled. Currently, that is the safest option for the user, since we do not really know when the FSP configures PRMRR to which size depending on SGX status. Hence, use the previously implemented algorithm to find a valid PRMRR size with respect to the user-specified maximum.
As soon as we know more about PRMRR in general and when to set which size, the PRMRR Kconfig can be extended.
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/include/cpu/x86/mtrr.h M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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 M src/soc/intel/apollolake/cpu.c M src/soc/intel/apollolake/memmap.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/memmap.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/memmap.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/memmap.c 21 files changed, 73 insertions(+), 76 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/21
Hello Patrick Rudolph, 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/+/35799
to look at the new patch set (#22).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
PRMRR size can only be set when SGX is enabled. Currently, that is the safest option for the user, since we do not really know when the FSP configures PRMRR to which size depending on SGX status. Hence, use the previously implemented algorithm to find a valid PRMRR size with respect to the user-specified maximum.
As soon as we know more about PRMRR in general and when to set which size, the PRMRR Kconfig can be extended.
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/include/cpu/x86/mtrr.h M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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 M src/soc/intel/apollolake/cpu.c M src/soc/intel/apollolake/memmap.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/memmap.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/memmap.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/memmap.c 21 files changed, 75 insertions(+), 76 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/22
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 23: Patch Set 22 was rebased.
Hello Patrick Rudolph, 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/+/35799
to look at the new patch set (#24).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
PRMRR size can only be set when SGX is enabled. Currently, that is the safest option for the user, since we do not really know when the FSP configures PRMRR to which size depending on SGX status. Hence, use the previously implemented algorithm to find a valid PRMRR size with respect to the user-specified maximum.
As soon as we know more about PRMRR in general and when to set which size, the PRMRR Kconfig can be extended.
Todo: - make PRMRR size selectable with sgx enabled/disabled with respect to sgx requirements - rework commit message
Tested successfully on X11SSM-F
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/include/cpu/x86/mtrr.h M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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 M src/soc/intel/apollolake/cpu.c 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/romstage/fsp_params.c M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 21 files changed, 81 insertions(+), 81 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/24
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 28:
This change is ready for review.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 29:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/apollolake/m... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/apollolake/m... PS29, Line 39: #ifdef SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE Why wouldn't you provide a default value of 0 for this macro?
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/common/block... File src/soc/intel/common/block/sgx/Kconfig:
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/common/block... PS29, Line 40: depends on SOC_INTEL_CANNONLAKE_BASE || SOC_INTEL_ICELAKE So are we going to try and maintain this list here (and in multiple places?). It seems a little brittle.
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/common/block... PS29, Line 53: depends on !SOC_INTEL_COMMON_BLOCK_SGX_ENABLE Why is the 1MiB carved out from SGX_ENABLE Kconfig?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 31: Patch Set 30 was rebased.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 32: Patch Set 31 was rebased.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 33: Patch Set 32 was rebased.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 33:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/apollolake/m... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/apollolake/m... PS29, Line 39: #ifdef SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE
Why wouldn't you provide a default value of 0 for this macro?
if you had looked into get_max_prmrr_size(), you'd know ;) get_max_prmrr_size has a parameter `limit`, while limit=0 means "no limit"
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/common/block... File src/soc/intel/common/block/sgx/Kconfig:
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/common/block... PS29, Line 40: depends on SOC_INTEL_CANNONLAKE_BASE || SOC_INTEL_ICELAKE
So are we going to try and maintain this list here (and in multiple places?). […]
what do you mean with "and in multiple places"?
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/common/block... PS29, Line 53: depends on !SOC_INTEL_COMMON_BLOCK_SGX_ENABLE
Why is the 1MiB carved out from SGX_ENABLE Kconfig?
Because SGX depends on PRMRR >= 32 MiB
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/skylake/memma... File src/soc/intel/skylake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/skylake/memma... PS5, Line 89:
readability? can remove it if you insist
superseeded
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 33:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/5/src/mainboard/intel/glkrvp/... File src/mainboard/intel/glkrvp/Kconfig:
https://review.coreboot.org/c/coreboot/+/35799/5/src/mainboard/intel/glkrvp/... PS5, Line 89: config SOC_INTEL_COMMON_BLOCK_SGX_ENABLE : bool : default y
Sorry, I don't get what you mean. sgx was enabled in the devicetree (yes, mb specific) before. now it gets enabled via the new Kconfig setting in the boards Kconfig (the mb specific one). I don't want to change the default behaviour as I don't know if SGX is working on all other boards.
Not sure what my train of thought was here. Is enabling SGX something that should be done by default on this platform or not? anyway... go ahead.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 33:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/apollolake/m... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/apollolake/m... PS29, Line 39: #ifdef SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE
if you had looked into get_max_prmrr_size(), you'd know ;) get_max_prmrr_size has a parameter `limit […]
Well, we could return 0 for limit=0... and if we ever needed "unlimited", this could be added for limit=-1
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 34: Patch Set 33 was rebased.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 34:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/5/src/mainboard/intel/glkrvp/... File src/mainboard/intel/glkrvp/Kconfig:
https://review.coreboot.org/c/coreboot/+/35799/5/src/mainboard/intel/glkrvp/... PS5, Line 89: config SOC_INTEL_COMMON_BLOCK_SGX_ENABLE : bool : default y
Sorry, I don't get what you mean. sgx was enabled in the devicetree (yes, mb specific) before. […]
good question... there is no benefit in doing so if the user does not use it explicitely; I'd be ok with disabling it per default instead
Hello Aaron Durbin, 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/+/35799
to look at the new patch set (#35).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
The option is implemented as a maximum value. At runtime the previously added algorithm is used to find a valid PRMRR size with respect to the user-specified maximum.
Tested successfully on X11SSM-F
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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 M src/soc/intel/apollolake/cpu.c 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/romstage/fsp_params.c M src/soc/intel/common/block/include/intelblocks/sgx.h M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 21 files changed, 96 insertions(+), 82 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/35
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 35.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 35:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/apollolake/m... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/apollolake/m... PS29, Line 39: #ifdef SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE
Well, we could return 0 for limit=0... […]
But the way you are constraining with Kconfig 'no limit' is impossible. This sequence of #ifdef is pretty ugly to accommodate properly. I would hope we wouldn't have to touch all this code.
One could even take it so far as to not have a get_max_prmrr_size() that takes a parameter because you ensuring you are providing the size through a macro based on Kconfig. In such a scenario when would we ever need a get_max_prmrr_size() with a parameter when it could just be get_prmrr_size() ?
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/common/block... File src/soc/intel/common/block/sgx/Kconfig:
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/common/block... PS29, Line 40: depends on SOC_INTEL_CANNONLAKE_BASE || SOC_INTEL_ICELAKE
what do you mean with "and in multiple places"?
Well, it looks like we keeping the dep list here and above on line 24. But regardless someone needs to append more conditions for every SoC, correct? Maybe I'm over thinking it, but it looks fairly brittle and another touch point for adding a new SoC.
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/common/block... PS29, Line 53: depends on !SOC_INTEL_COMMON_BLOCK_SGX_ENABLE
Because SGX depends on PRMRR >= 32 MiB
Please add a comment to that effect.
Hello Aaron Durbin, 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/+/35799
to look at the new patch set (#37).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
The option is implemented as a maximum value. At runtime the previously added algorithm is used to find a valid PRMRR size with respect to the user-specified maximum.
Tested successfully on X11SSM-F
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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 M src/soc/intel/apollolake/cpu.c 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/romstage/fsp_params.c M src/soc/intel/common/block/include/intelblocks/sgx.h M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/romstage/romstage.c 21 files changed, 96 insertions(+), 82 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/37
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 37.
(2 comments)
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/common/block... File src/soc/intel/common/block/sgx/Kconfig:
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/common/block... PS29, Line 40: depends on SOC_INTEL_CANNONLAKE_BASE || SOC_INTEL_ICELAKE
Well, it looks like we keeping the dep list here and above on line 24. […]
Yeah, I found a better solution for this by adding a "unlimited" option, so we don't need to maintain this; see latest patchset
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/common/block... PS29, Line 53: depends on !SOC_INTEL_COMMON_BLOCK_SGX_ENABLE
Please add a comment to that effect.
ok, done
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 37:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/apollolake/m... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/apollolake/m... PS29, Line 39: #ifdef SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE
But the way you are constraining with Kconfig 'no limit' is impossible. […] But the way you are constraining with Kconfig 'no limit' is impossible. This sequence of #ifdef is pretty ugly to accommodate properly. I would hope we wouldn't have to touch all this code.
I have redone this now; check again, please
One could even take it so far as to not have a get_max_prmrr_size() that takes a parameter because you ensuring you are providing the size through a macro based on Kconfig. In such a scenario when would we ever need a get_max_prmrr_size() with a parameter when it could just be get_prmrr_size() ?
Well, the problem is, that the user may select a wrong value, that is not supported. At Kconfig we can't know what the soc really supports
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 37:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/apollolake/m... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/apollolake/m... PS29, Line 39: #ifdef SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE
One could even take it so far as to not have a get_max_prmrr_size() that takes a parameter because you ensuring you are providing the size through a macro based on Kconfig. In such a scenario when would we ever need a get_max_prmrr_size() with a parameter when it could just be get_prmrr_size() ?
Well, the problem is, that the user may select a wrong value, that is not supported. At Kconfig we can't know what the soc really supports
Sure, and the runtime checks it. However, that's orthogonal to providing a macro that correlates to a value and where that value is inserted from a call site. Right now, you are open coding the use of a macro when this code really shouldn't be bothered with anything aside from the returned value of the prmrr size. i.e. this code doesn't care about the selected Kconfig value.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 37:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/37/src/soc/intel/apollolake/m... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/37/src/soc/intel/apollolake/m... PS37, Line 38: #ifdef SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE I think this guard can be removed. Plus I think there's no need for these callers to know about SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE. As noted in previous comments I think that complication can be handled entirely within the compilation unit supporting a get_prmrr_size(void) implementation.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 38.
Hello Aaron Durbin, 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/+/35799
to look at the new patch set (#38).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
The PRMRR size Kconfig has been implemented as a maximum value. At runtime the final PRMRR size gets selected by checking the supported values in MSR_PRMRR_VALID_CONFIG and trying to select the value nearest to the chosen one.
When "Maximum" is chosen, the highest possibly value from the MSR gets used. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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/cpu.c 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/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/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/romstage/romstage.c 24 files changed, 130 insertions(+), 83 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/38
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 38:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/apollolake/m... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/apollolake/m... PS29, Line 39: #ifdef SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE
One could even take it so far as to not have a get_max_prmrr_size() that takes a parameter becaus […]
Done; completely reworked
https://review.coreboot.org/c/coreboot/+/35799/37/src/soc/intel/apollolake/m... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/37/src/soc/intel/apollolake/m... PS37, Line 38: #ifdef SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE
I think this guard can be removed. […]
Done
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/common/block... File src/soc/intel/common/block/sgx/Kconfig:
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/common/block... PS29, Line 40: depends on SOC_INTEL_CANNONLAKE_BASE || SOC_INTEL_ICELAKE
Yeah, I found a better solution for this by adding a "unlimited" option, so we don't need to maintai […]
Done
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 38:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/5/src/mainboard/intel/glkrvp/... File src/mainboard/intel/glkrvp/Kconfig:
https://review.coreboot.org/c/coreboot/+/35799/5/src/mainboard/intel/glkrvp/... PS5, Line 89: config SOC_INTEL_COMMON_BLOCK_SGX_ENABLE : bool : default y
good question... […]
Ah, this here was added to keep the current behaviour (sgx was enabled in the devtree before); the soc default already is disabled
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 38:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/38//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35799/38//COMMIT_MSG@21 PS38, Line 21: Tested successfully on X11SSM-F need to retest after this rework
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#39).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
The PRMRR size Kconfig has been implemented as a maximum value. At runtime the final PRMRR size gets selected by checking the supported values in MSR_PRMRR_VALID_CONFIG and trying to select the value nearest to the chosen one.
When "Maximum" is chosen, the highest possibly value from the MSR gets used. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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/cpu.c 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/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/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/romstage/romstage.c 24 files changed, 130 insertions(+), 83 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/39
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 39.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 40.
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#40).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
The PRMRR size Kconfig has been implemented as a maximum value. At runtime the final PRMRR size gets selected by checking the supported values in MSR_PRMRR_VALID_CONFIG and trying to select the value nearest to the chosen one.
When "Maximum" is chosen, the highest possibly value from the MSR gets used. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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/cpu.c 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/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/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/romstage/romstage.c 24 files changed, 130 insertions(+), 83 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/40
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 40:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/40/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35799/40/src/soc/intel/common/block... PS40, Line 347: if (0 < valid_size <= CONFIG_SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE) Comparisons should place the constant on the right side of the test
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 41.
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#41).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
The PRMRR size Kconfig has been implemented as a maximum value. At runtime the final PRMRR size gets selected by checking the supported values in MSR_PRMRR_VALID_CONFIG and trying to select the value nearest to the chosen one.
When "Maximum" is chosen, the highest possibly value from the MSR gets used. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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/cpu.c 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/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/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/romstage/romstage.c 24 files changed, 131 insertions(+), 83 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/41
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 42: Patch Set 41 was rebased.
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#43).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
The PRMRR size Kconfig has been implemented as a maximum value. At runtime the final PRMRR size gets selected by checking the supported values in MSR_PRMRR_VALID_CONFIG and trying to select the value nearest to the chosen one.
When "Maximum" is chosen, the highest possibly value from the MSR gets used. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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/cpu.c 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/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/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/romstage/romstage.c 24 files changed, 131 insertions(+), 83 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/43
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 43.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 44.
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#44).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
The PRMRR size Kconfig has been implemented as a maximum value. At runtime the final PRMRR size gets selected by checking the supported values in MSR_PRMRR_VALID_CONFIG and trying to select the value nearest to the chosen one.
When "Maximum" is chosen, the highest possibly value from the MSR gets used. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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/cpu.c 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/romstage/fsp_params.c M src/soc/intel/common/block/cpu/Makefile.inc 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/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/romstage/romstage.c 25 files changed, 132 insertions(+), 83 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/44
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 44:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35799/44/src/soc/intel/apollolake/m... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/44/src/soc/intel/apollolake/m... PS44, Line 37: if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE)) This guard should be removed like the rest of the files.
https://review.coreboot.org/c/coreboot/+/35799/44/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35799/44/src/soc/intel/common/block... PS44, Line 347: PRMRR_SIZE Nit. This Kconfig variable is actually in MiB. Name it accordingly?
https://review.coreboot.org/c/coreboot/+/35799/44/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cpulib.h:
https://review.coreboot.org/c/coreboot/+/35799/44/src/soc/intel/common/block... PS44, Line 165: the a Just 'the'.
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#45).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
The PRMRR size Kconfig has been implemented as a maximum value. At runtime the final PRMRR size gets selected by checking the supported values in MSR_PRMRR_VALID_CONFIG and trying to select the value nearest to the chosen one.
When "Maximum" is chosen, the highest possibly value from the MSR gets used. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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/cpu.c 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/romstage/fsp_params.c M src/soc/intel/common/block/cpu/Makefile.inc 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/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/romstage/romstage.c 25 files changed, 132 insertions(+), 83 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/45
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 45:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/44/src/soc/intel/apollolake/m... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/44/src/soc/intel/apollolake/m... PS44, Line 37: if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE))
This guard should be removed like the rest of the files.
why?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 45:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/44/src/soc/intel/apollolake/m... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/44/src/soc/intel/apollolake/m... PS44, Line 37: if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE))
why?
It's not necessary, and it's more consistent with the rest of the code where it isn't guarded.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 46: Patch Set 45 was rebased.
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#47).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
The PRMRR size Kconfig has been implemented as a maximum value. At runtime the final PRMRR size gets selected by checking the supported values in MSR_PRMRR_VALID_CONFIG and trying to select the value nearest to the chosen one.
When "Maximum" is chosen, the highest possibly value from the MSR gets used. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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/cpu.c 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/romstage/fsp_params.c M src/soc/intel/common/block/cpu/Makefile.inc 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/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/romstage/romstage.c 25 files changed, 130 insertions(+), 82 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/47
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 47.
(1 comment)
https://review.coreboot.org/c/coreboot/+/35799/44/src/soc/intel/apollolake/m... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/44/src/soc/intel/apollolake/m... PS44, Line 37: if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE))
It's not necessary, and it's more consistent with the rest of the code where it isn't guarded.
oops, yeah ofc. I read something completely different... it's late... you know that... ;) thx
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 47: Code-Review+2
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#48).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
The PRMRR size Kconfig has been implemented as a maximum value. At runtime the final PRMRR size gets selected by checking the supported values in MSR_PRMRR_VALID_CONFIG and trying to select the value nearest to the chosen one.
When "Maximum" is chosen, the highest possibly value from the MSR gets used. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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/cpu.c 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/romstage/fsp_params.c M src/soc/intel/common/block/cpu/Makefile.inc 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/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/romstage/romstage.c 25 files changed, 130 insertions(+), 82 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/48
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 48: New patch set was added with same tree, parent, and commit message as Patch Set 47.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 49: Patch Set 48 was rebased.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 51:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35799/38//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35799/38//COMMIT_MSG@21 PS38, Line 21: Tested successfully on X11SSM-F
need to retest after this rework
Done
https://review.coreboot.org/c/coreboot/+/35799/44/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35799/44/src/soc/intel/common/block... PS44, Line 347: PRMRR_SIZE
Nit. This Kconfig variable is actually in MiB. […]
IMHO this thing is already long enough... a `grep -r _SIZE src/` showed many Kconfig vars that don't have that and not one that has it
https://review.coreboot.org/c/coreboot/+/35799/44/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cpulib.h:
https://review.coreboot.org/c/coreboot/+/35799/44/src/soc/intel/common/block... PS44, Line 165: the a
Just 'the'.
Done
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 51: Code-Review+1
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 52: Patch Set 51 was rebased.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 53: Patch Set 52 was rebased.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 55: Patch Set 54 was rebased.
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#56).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
The PRMRR size Kconfig has been implemented as a maximum value. At runtime the final PRMRR size gets selected by checking the supported values in MSR_PRMRR_VALID_CONFIG and trying to select the value nearest to the chosen one.
When "Maximum" is chosen, the highest possibly value from the MSR gets used. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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/cpu.c 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/romstage/fsp_params.c M src/soc/intel/common/block/cpu/Makefile.inc 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/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/romstage/romstage.c 25 files changed, 129 insertions(+), 82 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/56
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 57: Patch Set 56 was rebased.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 58.
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#58).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
The PRMRR size Kconfig has been implemented as a maximum value. At runtime the final PRMRR size gets selected by checking the supported values in MSR_PRMRR_VALID_CONFIG and trying to select the value nearest to the chosen one.
When "Maximum" is chosen, the highest possibly value from the MSR gets used. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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/cpu.c 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/romstage/fsp_params.c M src/soc/intel/common/block/cpu/Makefile.inc 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/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/romstage/romstage.c 25 files changed, 130 insertions(+), 85 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/58
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#59).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
The PRMRR size Kconfig has been implemented as a maximum value. At runtime the final PRMRR size gets selected by checking the supported values in MSR_PRMRR_VALID_CONFIG and trying to select the value nearest to the chosen one.
When "Maximum" is chosen, the highest possibly value from the MSR gets used. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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/cpu.c 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/romstage/fsp_params.c M src/soc/intel/common/block/cpu/Makefile.inc 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/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/romstage/romstage.c 25 files changed, 130 insertions(+), 85 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/59
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 59: New patch set was added with same tree, parent, and commit message as Patch Set 58.
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#60).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
The PRMRR size Kconfig has been implemented as a maximum value. At runtime the final PRMRR size gets selected by checking the supported values in MSR_PRMRR_VALID_CONFIG and trying to select the value nearest to the chosen one.
When "Maximum" is chosen, the highest possibly value from the MSR gets used. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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/cpu.c 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/romstage/fsp_params.c M src/soc/intel/common/block/cpu/Makefile.inc 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/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/romstage/romstage.c 25 files changed, 130 insertions(+), 85 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/60
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 61:
This change is ready for review.
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35799
to look at the new patch set (#62).
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
The PRMRR size Kconfig has been implemented as a maximum value. At runtime the final PRMRR size gets selected by checking the supported values in MSR_PRMRR_VALID_CONFIG and trying to select the value nearest to the chosen one.
When "Maximum" is chosen, the highest possibly value from the MSR gets used. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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/cpu.c 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/romstage/fsp_params.c M src/soc/intel/common/block/cpu/Makefile.inc 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/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/romstage/romstage.c 25 files changed, 130 insertions(+), 82 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35799/62
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 62: New patch set was added with same tree, parent, and commit message as Patch Set 61.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 62: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 63: Code-Review+2
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Uploaded patch set 64: Patch Set 63 was rebased.
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig
The devicetree is not made for user-choosable options, thus introduce Kconfig options for both SGX and the corresponding PRMRR size.
The PRMRR size Kconfig has been implemented as a maximum value. At runtime the final PRMRR size gets selected by checking the supported values in MSR_PRMRR_VALID_CONFIG and trying to select the value nearest to the chosen one.
When "Maximum" is chosen, the highest possibly value from the MSR gets used. When a too strict limit is set, coreboot will die, printing an error message.
Tested successfully on X11SSM-F
Change-Id: I5f08e85898304bba6680075ca5d6bce26aef9a4d Signed-off-by: Michael Niewöhner foss@mniewoehner.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/35799 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de --- M src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb M src/mainboard/intel/glkrvp/Kconfig 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/cpu.c 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/romstage/fsp_params.c M src/soc/intel/common/block/cpu/Makefile.inc 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/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c M src/soc/intel/icelake/chip.h M src/soc/intel/icelake/romstage/fsp_params.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip.h M src/soc/intel/skylake/cpu.c M src/soc/intel/skylake/romstage/romstage.c 25 files changed, 130 insertions(+), 82 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb b/src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb index bcad954..d77633d 100644 --- a/src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb +++ b/src/mainboard/google/dragonegg/variants/baseboard/devicetree.cb @@ -44,8 +44,6 @@ # EC memory map range is 0x900-0x9ff register "gen3_dec" = "0x00fc0901"
- register "PrmrrSize" = "0x10000000" - register "PcieRpEnable[0]" = "1" register "PcieRpEnable[1]" = "1" register "PcieRpEnable[2]" = "1" diff --git a/src/mainboard/intel/glkrvp/Kconfig b/src/mainboard/intel/glkrvp/Kconfig index 3380762..ebb5a3a 100644 --- a/src/mainboard/intel/glkrvp/Kconfig +++ b/src/mainboard/intel/glkrvp/Kconfig @@ -86,4 +86,8 @@ bool "Is this RVP1?" default n
+config SOC_INTEL_COMMON_BLOCK_SGX_ENABLE + bool + default y + endif # BOARD_INTEL_GLKRVP diff --git a/src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb b/src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb index d3d0b00..c5ad27d 100644 --- a/src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb +++ b/src/mainboard/intel/glkrvp/variants/baseboard/devicetree.cb @@ -97,14 +97,6 @@ # Minimum SLP S3 assertion width 28ms. register "slp_s3_assertion_width_usecs" = "28000"
- register "sgx_enable" = "1" - - # PRMRR size options - # 0x02000000 - 32MiB - # 0x04000000 - 64MiB - # 0x08000000 - 128MiB - register "PrmrrSize" = "128 * MiB" - register "pnp_settings" = "PNP_PERF_POWER"
device domain 0 on diff --git a/src/mainboard/intel/icelake_rvp/variants/icl_u/devicetree.cb b/src/mainboard/intel/icelake_rvp/variants/icl_u/devicetree.cb index 6d7fad7..12acced 100644 --- a/src/mainboard/intel/icelake_rvp/variants/icl_u/devicetree.cb +++ b/src/mainboard/intel/icelake_rvp/variants/icl_u/devicetree.cb @@ -48,8 +48,6 @@ register "PchHdaDspEnable" = "1" register "PchHdaAudioLinkHda" = "1"
- register "PrmrrSize" = "0x10000000" - register "PcieRpEnable[0]" = "1" register "PcieRpEnable[1]" = "1" register "PcieRpEnable[2]" = "1" diff --git a/src/mainboard/intel/icelake_rvp/variants/icl_y/devicetree.cb b/src/mainboard/intel/icelake_rvp/variants/icl_y/devicetree.cb index 4f41308..b12c0f7 100644 --- a/src/mainboard/intel/icelake_rvp/variants/icl_y/devicetree.cb +++ b/src/mainboard/intel/icelake_rvp/variants/icl_y/devicetree.cb @@ -48,8 +48,6 @@ register "PchHdaDspEnable" = "1" register "PchHdaAudioLinkHda" = "1"
- register "PrmrrSize" = "0x10000000" - register "PcieRpEnable[0]" = "1" register "PcieRpEnable[1]" = "1" register "PcieRpEnable[2]" = "1" diff --git a/src/mainboard/supermicro/x11-lga1151-series/devicetree.cb b/src/mainboard/supermicro/x11-lga1151-series/devicetree.cb index 1b9dc27..b58fbf1 100644 --- a/src/mainboard/supermicro/x11-lga1151-series/devicetree.cb +++ b/src/mainboard/supermicro/x11-lga1151-series/devicetree.cb @@ -17,10 +17,6 @@ register "Device4Enable" = "1" register "SaGv" = "SaGv_Disabled"
- # Enable SGX - register "sgx_enable" = "1" - register "PrmrrSize" = "128 * MiB" - register "pirqa_routing" = "PCH_IRQ11" register "pirqb_routing" = "PCH_IRQ10" register "pirqc_routing" = "PCH_IRQ11" diff --git a/src/soc/intel/apollolake/Makefile.inc b/src/soc/intel/apollolake/Makefile.inc index 5530e5c..7655d5a 100644 --- a/src/soc/intel/apollolake/Makefile.inc +++ b/src/soc/intel/apollolake/Makefile.inc @@ -1,5 +1,6 @@ ifeq ($(CONFIG_SOC_INTEL_APOLLOLAKE),y)
+subdirs-y += ../../../cpu/intel/common subdirs-y += ../../../cpu/intel/microcode subdirs-y += ../../../cpu/intel/turbo subdirs-y += ../../../cpu/x86/lapic diff --git a/src/soc/intel/apollolake/chip.h b/src/soc/intel/apollolake/chip.h index 85cfff9..e5045d0 100644 --- a/src/soc/intel/apollolake/chip.h +++ b/src/soc/intel/apollolake/chip.h @@ -139,18 +139,6 @@ /* GPIO SD card detect pin */ unsigned int sdcard_cd_gpio;
- /* PRMRR size setting with three options - * 0x02000000 - 32MiB - * 0x04000000 - 64MiB - * 0x08000000 - 128MiB */ - uint32_t PrmrrSize; - - /* Enable SGX feature. - * Enabling SGX feature is 2 step process, - * (1) set sgx_enable = 1 - * (2) set PrmrrSize to supported size */ - uint8_t sgx_enable; - /* Select PNP Settings. * (0) Performance, * (1) Power diff --git a/src/soc/intel/apollolake/cpu.c b/src/soc/intel/apollolake/cpu.c index 6e826b8..0b9466c 100644 --- a/src/soc/intel/apollolake/cpu.c +++ b/src/soc/intel/apollolake/cpu.c @@ -72,13 +72,10 @@
void soc_core_init(struct device *cpu) { - config_t *conf = config_of_soc(); - /* Clear out pending MCEs */ /* TODO(adurbin): Some of these banks are core vs package scope. For now every CPU clears every bank. */ - if ((CONFIG(SOC_INTEL_COMMON_BLOCK_SGX) && conf->sgx_enable) || - acpi_get_sleep_type() == ACPI_S5) + if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE) || acpi_get_sleep_type() == ACPI_S5) mca_configure();
/* Set core MSRs */ @@ -91,7 +88,7 @@ enable_pm_timer_emulation();
/* Configure Core PRMRR for SGX. */ - if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX) && conf->sgx_enable) + if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE)) prmrr_core_configure();
/* Set Max Non-Turbo ratio if RAPL is disabled. */ @@ -255,11 +252,9 @@
static void post_mp_init(void) { - config_t *conf = config_of_soc(); - smm_southbridge_enable(PWRBTN_EN | GBL_EN);
- if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX) && conf->sgx_enable) + if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE)) mp_run_on_all_cpus(sgx_configure, NULL); }
diff --git a/src/soc/intel/apollolake/memmap.c b/src/soc/intel/apollolake/memmap.c index 567ff1e..de6a7d1 100644 --- a/src/soc/intel/apollolake/memmap.c +++ b/src/soc/intel/apollolake/memmap.c @@ -16,23 +16,20 @@ */
#include <cbmem.h> +#include <intelblocks/cpulib.h> #include <soc/systemagent.h>
#include "chip.h"
void *cbmem_top_chipset(void) { - const config_t *config; void *tolum = (void *)sa_get_tseg_base();
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; + 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..17afdd1 100644 --- a/src/soc/intel/cannonlake/chip.h +++ b/src/soc/intel/cannonlake/chip.h @@ -261,12 +261,7 @@
/* Enable C6 DRAM */ uint8_t enable_c6dram; - /* - * PRMRR size setting with below options - * 0x00100000 - 1MiB - * 0x02000000 - 32MiB and beyond - */ - uint32_t PrmrrSize; + uint8_t PmTimerDisabled;
/* diff --git a/src/soc/intel/cannonlake/romstage/fsp_params.c b/src/soc/intel/cannonlake/romstage/fsp_params.c index 3ba997d..996c135 100644 --- a/src/soc/intel/cannonlake/romstage/fsp_params.c +++ b/src/soc/intel/cannonlake/romstage/fsp_params.c @@ -17,6 +17,7 @@ #include <cpu/x86/msr.h> #include <console/console.h> #include <fsp/util.h> +#include <intelblocks/cpulib.h> #include <intelblocks/pmclib.h> #include <soc/iomap.h> #include <soc/msr.h> @@ -48,7 +49,7 @@ mask |= (1 << i); } m_cfg->PcieRpEnableMask = mask; - m_cfg->PrmrrSize = config->PrmrrSize; + m_cfg->PrmrrSize = get_prmrr_size(); m_cfg->EnableC6Dram = config->enable_c6dram; #if CONFIG(SOC_INTEL_COMETLAKE) m_cfg->SerialIoUartDebugControllerNumber = CONFIG_UART_FOR_CONSOLE; diff --git a/src/soc/intel/common/block/cpu/Makefile.inc b/src/soc/intel/common/block/cpu/Makefile.inc index a6c4f37..f263053 100644 --- a/src/soc/intel/common/block/cpu/Makefile.inc +++ b/src/soc/intel/common/block/cpu/Makefile.inc @@ -7,6 +7,7 @@ romstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_CPU) += cpulib.c
postcar-$(CONFIG_SOC_INTEL_COMMON_BLOCK_CAR) += car/exit_car.S +postcar-$(CONFIG_SOC_INTEL_COMMON_BLOCK_CPU) += cpulib.c postcar-$(CONFIG_FSP_CAR) += car/exit_car_fsp.S
ramstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_CPU) += cpulib.c diff --git a/src/soc/intel/common/block/cpu/cpulib.c b/src/soc/intel/common/block/cpu/cpulib.c index 71e4dbf..89732f1 100644 --- a/src/soc/intel/common/block/cpu/cpulib.c +++ b/src/soc/intel/common/block/cpu/cpulib.c @@ -325,3 +325,44 @@ { msr_set_bit(MSR_LT_CONTROL, LT_CONTROL_LOCK_BIT); } + +int get_prmrr_size(void) +{ + msr_t msr; + int i; + int valid_size; + + if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_DISABLED)) { + printk(BIOS_DEBUG, "PRMRR disabled by config.\n"); + return 0; + } + + msr = rdmsr(MSR_PRMRR_VALID_CONFIG); + if (!msr.lo) { + printk(BIOS_WARNING, "PRMRR not supported.\n"); + return 0; + } + + printk(BIOS_DEBUG, "MSR_PRMRR_VALID_CONFIG = 0x%08x\n", msr.lo); + + /* find the first (greatest) value that is lower than or equal to the selected size */ + for (i = 8; i >= 0; i--) { + valid_size = msr.lo & (1 << i); + + if (valid_size && valid_size <= CONFIG_SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE) + break; + else if (i == 0) + valid_size = 0; + } + + /* die if we could not find a valid size within the limit */ + if (!valid_size) + die("Unsupported PRMRR size limit %i MiB, check your config!\n", + CONFIG_SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE); + + printk(BIOS_DEBUG, "PRMRR size set to %i MiB\n", valid_size); + + valid_size *= MiB; + + return valid_size; +} diff --git a/src/soc/intel/common/block/include/intelblocks/cpulib.h b/src/soc/intel/common/block/include/intelblocks/cpulib.h index 1aa88e1..a422094 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 @@ -164,4 +165,7 @@ /* Lock chipset memory registers to protect SMM */ void cpu_lt_lock_memory(void *unused);
+/* Get the a supported PRMRR size in bytes with respect users choice */ +int get_prmrr_size(void); + #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 3e67fd7..8902d09 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/common/block/sgx/Kconfig b/src/soc/intel/common/block/sgx/Kconfig index 026c6af..6e8323f 100644 --- a/src/soc/intel/common/block/sgx/Kconfig +++ b/src/soc/intel/common/block/sgx/Kconfig @@ -4,9 +4,7 @@ select CPU_INTEL_COMMON_HYPERTHREADING default n help - Software Guard eXtension(SGX) Feature. Intel SGX is a set of new CPU - instructions that can be used by applications to set aside private - regions of code and data. + Intel Processor common SGX support
config SOC_INTEL_COMMON_BLOCK_SGX_LOCK_MEMORY bool @@ -14,3 +12,61 @@ 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" + depends on SOC_INTEL_COMMON_BLOCK_SGX + 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 + default 256 if SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE_256MB + default 128 if SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE_128MB + default 64 if SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE_64MB + default 32 if SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE_32MB + default 1 if SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE_1MB + +choice + prompt "PRMRR size" + default SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE_MAX if SOC_INTEL_COMMON_BLOCK_SGX_ENABLE + default SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_DISABLED if !SOC_INTEL_COMMON_BLOCK_SGX_ENABLE + help + PRMRR (Protected Memory Range) is the space in RAM that is used to provide a protected + memory area (e.g. for the Intel SGX Secure Enclaves). The memory region is accessible + only by the processor itself to protect the data from unauthorized access. + + This option selects the maximum size that gets reserved. Depending on the SoC a lower, + compatible value may be chosen at runtime as not all values are supported on all + families. + +config SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE_MAX + bool "Maximum" + +config SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE_256MB + bool "256 MiB" + +config SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE_128MB + bool "128 MiB" + +config SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE_64MB + bool "64 MiB" + +config SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE_32MB + bool "32 MiB" + +config SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE_1MB + depends on !SOC_INTEL_COMMON_BLOCK_SGX_ENABLE # SGX depends on PRMRR >= 32 MiB + bool "1 MiB" + +config SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_DISABLED + depends on !SOC_INTEL_COMMON_BLOCK_SGX_ENABLE # SGX depends on PRMRR >= 32 MiB + bool "Disabled" + +endchoice diff --git a/src/soc/intel/common/block/sgx/sgx.c b/src/soc/intel/common/block/sgx/sgx.c index 842eb43..6f0cfd8 100644 --- a/src/soc/intel/common/block/sgx/sgx.c +++ b/src/soc/intel/common/block/sgx/sgx.c @@ -206,7 +206,7 @@ {
if (!is_sgx_supported() || !is_prmrr_set()) { - printk(BIOS_ERR, "SGX: pre-conditions not met\n"); + printk(BIOS_ERR, "SGX: not supported or pre-conditions not met\n"); return; }
diff --git a/src/soc/intel/icelake/chip.h b/src/soc/intel/icelake/chip.h index fc9341c..ec625a0 100644 --- a/src/soc/intel/icelake/chip.h +++ b/src/soc/intel/icelake/chip.h @@ -206,13 +206,9 @@
/* Enable C6 DRAM */ uint8_t enable_c6dram; - /* - * PRMRR size setting with below options - * 0x00100000 - 1MiB - * 0x02000000 - 32MiB and beyond - */ - uint32_t PrmrrSize; + uint8_t PmTimerDisabled; + /* Desired platform debug type. */ enum { DebugConsent_Disabled, diff --git a/src/soc/intel/icelake/romstage/fsp_params.c b/src/soc/intel/icelake/romstage/fsp_params.c index 5bf3421..1f99604 100644 --- a/src/soc/intel/icelake/romstage/fsp_params.c +++ b/src/soc/intel/icelake/romstage/fsp_params.c @@ -16,6 +16,7 @@ #include <assert.h> #include <console/console.h> #include <fsp/util.h> +#include <intelblocks/cpulib.h> #include <soc/iomap.h> #include <soc/pci_devs.h> #include <soc/romstage.h> @@ -60,7 +61,7 @@ mask |= (1 << i); } m_cfg->PcieRpEnableMask = mask; - m_cfg->PrmrrSize = config->PrmrrSize; + m_cfg->PrmrrSize = get_prmrr_size(); m_cfg->EnableC6Dram = config->enable_c6dram; /* Disable BIOS Guard */ m_cfg->BiosGuard = 0; diff --git a/src/soc/intel/skylake/acpi.c b/src/soc/intel/skylake/acpi.c index 2af5a53..332f797 100644 --- a/src/soc/intel/skylake/acpi.c +++ b/src/soc/intel/skylake/acpi.c @@ -205,7 +205,7 @@ gnvs->u2we = config->usb2_wake_enable_bitmap; gnvs->u3we = config->usb3_wake_enable_bitmap;
- if (config->sgx_enable) + if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE)) sgx_fill_gnvs(gnvs); }
diff --git a/src/soc/intel/skylake/chip.h b/src/soc/intel/skylake/chip.h index 944315b..63626663 100644 --- a/src/soc/intel/skylake/chip.h +++ b/src/soc/intel/skylake/chip.h @@ -510,14 +510,6 @@ */ u8 SendVrMbxCmd;
- /* - * PRMRR size setting with three options - * 0x02000000 - 32MiB - * 0x04000000 - 64MiB - * 0x08000000 - 128MiB - */ - u32 PrmrrSize; - /* Enable/Disable host reads to PMC XRAM registers */ u8 PchPmPmcReadDisable;
@@ -576,9 +568,6 @@ u8 SlowSlewRateForGt; u8 SlowSlewRateForSa;
- /* Enable SGX feature */ - u8 sgx_enable; - /* Enable/Disable EIST * 1b - Enabled * 0b - Disabled diff --git a/src/soc/intel/skylake/cpu.c b/src/soc/intel/skylake/cpu.c index bfed528..080dba0 100644 --- a/src/soc/intel/skylake/cpu.c +++ b/src/soc/intel/skylake/cpu.c @@ -442,8 +442,6 @@ /* All CPUs including BSP will run the following function. */ void soc_core_init(struct device *cpu) { - config_t *conf = config_of_soc(); - /* Clear out pending MCEs */ /* TODO(adurbin): This should only be done on a cold boot. Also, some * of these banks are core vs package scope. For now every CPU clears @@ -479,7 +477,7 @@ enable_turbo();
/* Configure Core PRMRR for SGX. */ - if (conf->sgx_enable) + if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE)) prmrr_core_configure(); }
@@ -502,7 +500,6 @@ static void post_mp_init(void) { int ret = 0; - config_t *conf = config_of_soc();
/* Set Max Ratio */ cpu_set_max_ratio(); @@ -519,7 +516,7 @@
ret |= mp_run_on_all_cpus(vmx_configure, NULL);
- if (conf->sgx_enable) + if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE)) ret |= mp_run_on_all_cpus(sgx_configure, NULL);
ret |= mp_run_on_all_cpus(fc_lock_configure, NULL); diff --git a/src/soc/intel/skylake/romstage/romstage.c b/src/soc/intel/skylake/romstage/romstage.c index af89441..a72b261 100644 --- a/src/soc/intel/skylake/romstage/romstage.c +++ b/src/soc/intel/skylake/romstage/romstage.c @@ -22,6 +22,7 @@ #include <console/console.h> #include <device/pci_def.h> #include <fsp/util.h> +#include <intelblocks/cpulib.h> #include <intelblocks/pmclib.h> #include <memory_info.h> #include <smbios.h> @@ -237,7 +238,7 @@ 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);
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)