Change in coreboot[master]: soc/intel/common/block: Do not die if PRMRR size unsupported
Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45373 )
Change subject: soc/intel/common/block: Do not die if PRMRR size unsupported ......................................................................
soc/intel/common/block: Do not die if PRMRR size unsupported
If a given PRMRR size is not supported, do NOT brick people's devices. We don't do that when PRMRRs aren't even supported anyway.
Change-Id: Ib917be873aedbc5e789bb0894fca335b5ee9e2c2 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/common/block/cpu/cpulib.c 1 file changed, 4 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/45373/1
diff --git a/src/soc/intel/common/block/cpu/cpulib.c b/src/soc/intel/common/block/cpu/cpulib.c index 794c900..6ca7a53 100644 --- a/src/soc/intel/common/block/cpu/cpulib.c +++ b/src/soc/intel/common/block/cpu/cpulib.c @@ -369,10 +369,11 @@ 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", + if (!valid_size) { + printk(BIOS_WARNING, "Unsupported PRMRR size of %i MiB, check your config!\n", CONFIG_SOC_INTEL_COMMON_BLOCK_CPU_PRMRR_SIZE); + return 0; + }
printk(BIOS_DEBUG, "PRMRR size set to %i MiB\n", valid_size);
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45373 )
Change subject: soc/intel/common/block: Do not die if PRMRR size unsupported ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45373/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45373/1//COMMIT_MSG@10 PS1, Line 10: We don't do that when PRMRRs aren't even supported anyway. Reviewers, if you believe these lines are charged with too much frustration, feel free to suggest something else. Personally, I think it's not too bad, I am Weird.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45373 )
Change subject: soc/intel/common/block: Do not die if PRMRR size unsupported ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/45373/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45373/1//COMMIT_MSG@10 PS1, Line 10: We don't do that when PRMRRs aren't even supported anyway.
Reviewers, if you believe these lines are charged with too much frustration, feel free to suggest so […]
I don't quite get the statement, tbh. We don't brick when it's unsupported, so shouldn't brick if it is?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45373 )
Change subject: soc/intel/common/block: Do not die if PRMRR size unsupported ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45373/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45373/1//COMMIT_MSG@10 PS1, Line 10: We don't do that when PRMRRs aren't even supported anyway.
I don't quite get the statement, tbh. We don't brick when it's […]
We only brick when the PRMRR *size* is unsupported. Let me try to explain the difference in a weird way:
PRMRR_Disabled : exception; PRMRRs_Not_Supported : exception; PRMRR_Size_Unsupported : exception;
-- [...]
exception when PRMRR_Size_Unsupported => Die; when others => return 0;
I could put this in the commit message, but I'd get weird looks 👀
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45373 )
Change subject: soc/intel/common/block: Do not die if PRMRR size unsupported ......................................................................
Patch Set 1:
When EnableC6Dram=1 and the user selects an unsupported PRMRR size, the device will brick in runtime. I'm not sure if that is any better.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45373 )
Change subject: soc/intel/common/block: Do not die if PRMRR size unsupported ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45373/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/45373/1/src/soc/intel/common/block/... PS1, Line 373: BIOS_WARNING This is more an error than a warning, since programming PRMRR seems to be a *must* (see the check in chipsec), even when it's 0. Initially, that was the reason to die here.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45373 )
Change subject: soc/intel/common/block: Do not die if PRMRR size unsupported ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45373/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/45373/1/src/soc/intel/common/block/... PS1, Line 373: BIOS_WARNING
This is more an error than a warning, since programming PRMRR seems to be a *must* (see the check in […]
I wonder if there is any device where we currently hit that die(). that should only happen in an absolute error case, where die() would be right. when PRMRR is not supported by the soc, the code returns at line 357. that means die() can only be hit when the user selects a PRMRR size, that is too small. as of my knowlege, currently there is no soc that wouldn't support any of the settings, except the highest value 256MB, which is only supported by newer socs.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45373 )
Change subject: soc/intel/common/block: Do not die if PRMRR size unsupported ......................................................................
Patch Set 1:
(1 comment)
Patch Set 1:
When EnableC6Dram=1 and the user selects an unsupported PRMRR size, the device will brick in runtime. I'm not sure if that is any better.
This turned out to be wrong, since fsp sets PRMRR right, when EnableC6Dram=1
https://review.coreboot.org/c/coreboot/+/45373/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/45373/1/src/soc/intel/common/block/... PS1, Line 373: BIOS_WARNING
I wonder if there is any device where we currently hit that die(). […]
thinking about this again, just printing a warning or an error is ok, since the prmrr will result in size=0 then; so, ignore my second comment here
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45373 )
Change subject: soc/intel/common/block: Do not die if PRMRR size unsupported ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45373/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/45373/1/src/soc/intel/common/block/... PS1, Line 373: BIOS_WARNING
thinking about this again, just printing a warning or an error is ok, since the prmrr will result in […]
So what exactly is unresolved here?
If FSP doesn the right thing even with `0` passed, this would even be BIOS_NOTICE by definition.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45373 )
Change subject: soc/intel/common/block: Do not die if PRMRR size unsupported ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45373/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/45373/1/src/soc/intel/common/block/... PS1, Line 373: BIOS_WARNING
So what exactly is unresolved here? […]
ack, do as you wish
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45373 )
Change subject: soc/intel/common/block: Do not die if PRMRR size unsupported ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45373/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/45373/1/src/soc/intel/common/block/... PS1, Line 373: BIOS_WARNING
So what exactly is unresolved here? […]
I chose BIOS_WARNING here because that's what the "PRMRR not supported" case uses. If anything, both messages would need to be changed at once.
I'd rather do this on a separate patchset. I don't want to keep the tree in a known-broken state.
Angel Pons has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/45373 )
Change subject: soc/intel/common/block: Do not die if PRMRR size unsupported ......................................................................
Abandoned
Angel Pons has restored this change. ( https://review.coreboot.org/c/coreboot/+/45373 )
Change subject: soc/intel/common/block: Do not die if PRMRR size unsupported ......................................................................
Restored
Hello build bot (Jenkins), Nico Huber, Patrick Georgi, Michael Niewöhner, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45373
to look at the new patch set (#2).
Change subject: soc/intel/common/block: Do not die if PRMRR size unsupported ......................................................................
soc/intel/common/block: Do not die if PRMRR size unsupported
If a given PRMRR size is not supported, do NOT brick people's devices. We don't do that when PRMRRs aren't even supported anyway.
Change-Id: Ib917be873aedbc5e789bb0894fca335b5ee9e2c2 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/common/block/cpu/cpulib.c 1 file changed, 4 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/45373/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45373 )
Change subject: soc/intel/common/block: Do not die if PRMRR size unsupported ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45373/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/45373/1/src/soc/intel/common/block/... PS1, Line 373: BIOS_WARNING
I chose BIOS_WARNING here because that's what the "PRMRR not supported" case uses. […]
Tree isn't broken anymore. Kept BIOS_WARNING, as loglevel.h says it's "Bad configuration" (and this is, I think)
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45373 )
Change subject: soc/intel/common/block: Do not die if PRMRR size unsupported ......................................................................
Patch Set 2: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45373 )
Change subject: soc/intel/common/block: Do not die if PRMRR size unsupported ......................................................................
Patch Set 2: Code-Review+2
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45373 )
Change subject: soc/intel/common/block: Do not die if PRMRR size unsupported ......................................................................
Patch Set 2: Code-Review+2
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45373 )
Change subject: soc/intel/common/block: Do not die if PRMRR size unsupported ......................................................................
soc/intel/common/block: Do not die if PRMRR size unsupported
If a given PRMRR size is not supported, do NOT brick people's devices. We don't do that when PRMRRs aren't even supported anyway.
Change-Id: Ib917be873aedbc5e789bb0894fca335b5ee9e2c2 Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45373 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Michael Niewöhner foss@mniewoehner.de Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Matt DeVillier matt.devillier@gmail.com --- M src/soc/intel/common/block/cpu/cpulib.c 1 file changed, 4 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Matt DeVillier: 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 31f160a..fd39197 100644 --- a/src/soc/intel/common/block/cpu/cpulib.c +++ b/src/soc/intel/common/block/cpu/cpulib.c @@ -367,10 +367,11 @@ 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", + if (!valid_size) { + printk(BIOS_WARNING, "Unsupported PRMRR size of %i MiB, check your config!\n", CONFIG_SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE); + return 0; + }
printk(BIOS_DEBUG, "PRMRR size set to %i MiB\n", valid_size);
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45373 )
Change subject: soc/intel/common/block: Do not die if PRMRR size unsupported ......................................................................
Patch Set 3:
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/19620 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19619 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/19618 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19617 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/19616 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19624 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19623 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19622 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19621
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Matt DeVillier (Code Review)
-
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)