Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
soc/intel/common/block/cpu: Fix boot failure
This fixes CB:45043 "Guard options with if-blocks".
The code no longer returns if SGX is disabled, but as the PRMRR configuration is missing it runs into die().
Tested on Prodrive Hermes: Boots again into Linux.
Change-Id: I6d32ca32b1b53767b2db91305103cd532823a5ca Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/soc/intel/common/block/cpu/cpulib.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/45344/1
diff --git a/src/soc/intel/common/block/cpu/cpulib.c b/src/soc/intel/common/block/cpu/cpulib.c index 5b703cf..9cf7777 100644 --- a/src/soc/intel/common/block/cpu/cpulib.c +++ b/src/soc/intel/common/block/cpu/cpulib.c @@ -346,6 +346,9 @@ int i; int valid_size;
+ if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX)) + return 0; + if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_DISABLED)) { printk(BIOS_DEBUG, "PRMRR disabled by config.\n"); return 0;
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
Patch Set 1: Code-Review+2
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
Patch Set 1: Code-Review+2
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45344/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/45344/1/src/soc/intel/common/block/... PS1, Line 349: if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX)) : return 0; uhm, no. prmrr can exist without sgx
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45344/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/45344/1/src/soc/intel/common/block/... PS1, Line 349: if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX)) : return 0;
uhm, no. […]
CB:45345 would allow PRMRRs to be set without SGX
https://review.coreboot.org/c/coreboot/+/45344/1/src/soc/intel/common/block/... PS1, Line 352: if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_DISABLED)) { : printk(BIOS_DEBUG, "PRMRR disabled by config.\n"); : return 0; : } What's this for, then?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
Patch Set 1: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/45344/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/45344/1/src/soc/intel/common/block/... PS1, Line 349: if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX)) : return 0;
CB:45345 would allow PRMRRs to be set without SGX
Well, if PRMRR isn't SGX-specific, then it should be moved out of SGX Kconfig.
https://review.coreboot.org/c/coreboot/+/45344/1/src/soc/intel/common/block/... PS1, Line 352: if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_DISABLED)) { : printk(BIOS_DEBUG, "PRMRR disabled by config.\n"); : return 0; : }
What's this for, then?
It doesn't get set because CNL doesn't select SOC_INTEL_COMMON_BLOCK_SGX
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45344/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45344/1//COMMIT_MSG@9 PS1, Line 9: CB:45043 Please use `commit 1b89f5e`
Hello build bot (Jenkins), Patrick Georgi, Angel Pons, Patrick Rudolph, HAOUAS Elyes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45344
to look at the new patch set (#2).
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
soc/intel/common/block/cpu: Fix boot failure
This fixes CB:45043 "Guard options with if-blocks".
The code no longer returns if SGX is disabled, but as the PRMRR configuration is missing it runs into die().
Tested on Prodrive Hermes: Boots again into Linux.
Change-Id: I6d32ca32b1b53767b2db91305103cd532823a5ca Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/soc/intel/common/block/cpu/cpulib.c 1 file changed, 1 insertion(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/45344/2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45344/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/45344/1/src/soc/intel/common/block/... PS1, Line 349: if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX)) : return 0;
CB:45345 would allow PRMRRs to be set without SGX
Looking at the Kconfig PRMRR is always disabled when SGX disabled, so this condition is correct.
https://review.coreboot.org/c/coreboot/+/45344/1/src/soc/intel/common/block/... PS1, Line 352: if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_DISABLED)) { : printk(BIOS_DEBUG, "PRMRR disabled by config.\n"); : return 0; : }
What's this for, then?
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
Patch Set 2: Code-Review+2
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45344/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/45344/1/src/soc/intel/common/block/... PS1, Line 349: if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX)) : return 0;
Looking at the Kconfig PRMRR is always disabled when SGX disabled, so this condition is correct.
yes, *now* it does that, after guarding that with if's, but that was probably wrong already ;) we need to find a definitive answer if prmrr makes any sense without sgx
https://review.coreboot.org/c/coreboot/+/45344/1/src/soc/intel/common/block/... PS1, Line 352: if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_DISABLED)) { : printk(BIOS_DEBUG, "PRMRR disabled by config.\n"); : return 0; : }
What's this for, then?
you CAN disable prmrr
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
Patch Set 2: Code-Review+2
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
Patch Set 2: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/45344/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/45344/1/src/soc/intel/common/block/... PS1, Line 349: if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX)) : return 0;
yes, *now* it does that, after guarding that with if's, but that was probably wrong already ;) we ne […]
Intel doc# 329298-002 makes more or less clear that PRMRR can only be used by SGX
https://review.coreboot.org/c/coreboot/+/45344/1/src/soc/intel/common/block/... PS1, Line 352: if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_DISABLED)) { : printk(BIOS_DEBUG, "PRMRR disabled by config.\n"); : return 0; : }
What's this for, then? […]
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
Patch Set 2: Code-Review-1
I would prefer to move the PRMRR related Kconfigs out of the sgx/ dir instead. While this fixes one case (no SGX) it leaves another broken (PRMRR only for C6DRAM, whatever that is).
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
Patch Set 2: -Code-Review
CB:45345 (still not updated) will place the PRMRR Kconfig options where they should have always been.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
Patch Set 2: Code-Review+2
Sorry for the noise. It turned out that this function and the `PrmrrSize` UPD are secretly SGX specific. We might want to rename things to make that clear, but let's fix things first.
Nico Huber has uploaded a new patch set (#3) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
soc/intel/common/block/cpu: Fix boot failure
This fixes commit 1b89f5e "Guard options with if-blocks".
The code no longer returns if SGX is disabled, but as the PRMRR configuration is missing it runs into die().
Tested on Prodrive Hermes: Boots again into Linux.
Change-Id: I6d32ca32b1b53767b2db91305103cd532823a5ca Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/soc/intel/common/block/cpu/cpulib.c 1 file changed, 1 insertion(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/45344/3
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45344/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45344/1//COMMIT_MSG@9 PS1, Line 9: CB:45043
Please use `commit 1b89f5e`
Done
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
soc/intel/common/block/cpu: Fix boot failure
This fixes commit 1b89f5e "Guard options with if-blocks".
The code no longer returns if SGX is disabled, but as the PRMRR configuration is missing it runs into die().
Tested on Prodrive Hermes: Boots again into Linux.
Change-Id: I6d32ca32b1b53767b2db91305103cd532823a5ca Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45344 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Christian Walter christian.walter@9elements.com Reviewed-by: Michael Niewöhner foss@mniewoehner.de Reviewed-by: Nico Huber nico.h@gmx.de --- M src/soc/intel/common/block/cpu/cpulib.c 1 file changed, 1 insertion(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Christian Walter: Looks good to me, approved Michael Niewöhner: Looks good to me, approved
diff --git a/src/soc/intel/common/block/cpu/cpulib.c b/src/soc/intel/common/block/cpu/cpulib.c index 5b703cf..31f160a 100644 --- a/src/soc/intel/common/block/cpu/cpulib.c +++ b/src/soc/intel/common/block/cpu/cpulib.c @@ -346,10 +346,8 @@ int i; int valid_size;
- if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_DISABLED)) { - printk(BIOS_DEBUG, "PRMRR disabled by config.\n"); + if (!CONFIG(SOC_INTEL_COMMON_BLOCK_SGX)) return 0; - }
msr = rdmsr(MSR_PRMRR_VALID_CONFIG); if (!msr.lo) {
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
Patch Set 4:
Automatic boot test returned (PASS/FAIL/TOTAL): 8/1/9 "QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19407 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19406 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/19405 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19404 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/19403 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19411 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19410 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19409 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19408
Please note: This test is under development and might not be accurate at all!
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45344/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/45344/4/src/soc/intel/common/block/... PS4, Line 349: SOC_INTEL_COMMON_BLOCK_SGX shouldn't this be SOC_INTEL_COMMON_BLOCK_SGX_ENABLE ? SoC support != feature enabled
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45344/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/45344/4/src/soc/intel/common/block/... PS4, Line 349: SOC_INTEL_COMMON_BLOCK_SGX
shouldn't this be SOC_INTEL_COMMON_BLOCK_SGX_ENABLE ? […]
Good question. I think the original check would have covered that case, but it's gone. I'd add another check for SOC_INTEL_COMMON_BLOCK_SGX_ENABLE.
Checking for SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_DISABLED is a bad idea anyway, since it defaults to 0.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45344/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/45344/4/src/soc/intel/common/block/... PS4, Line 349: SOC_INTEL_COMMON_BLOCK_SGX
Checking for SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_DISABLED is a bad idea anyway, since it defaults to 0.
I don't get what you mean. That option was a bool, not the actual selected size. However, I agree that we should check for SGX feature and enablement
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45344/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/45344/4/src/soc/intel/common/block/... PS4, Line 349: SOC_INTEL_COMMON_BLOCK_SGX
Checking for SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_DISABLED is a bad idea anyway, since it defaults to […]
If SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_DISABLED isn't selected, it evaluates to zero. This can be because PRMRR size is something other than `Disabled`, or because `SOC_INTEL_COMMON_BLOCK_SGX` isn't selected at all.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45344 )
Change subject: soc/intel/common/block/cpu: Fix boot failure ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45344/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/45344/4/src/soc/intel/common/block/... PS4, Line 349: SOC_INTEL_COMMON_BLOCK_SGX
If SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_DISABLED isn't selected, it evaluates to zero. […]
oh indeed, thx
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Christian Walter (Code Review)
-
HAOUAS Elyes (Code Review)
-
Matt DeVillier (Code Review)
-
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)