Hello Joel Kitching,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/38417
to review the following change.
Change subject: vboot: Remove hard dependency on MISSING_BOARD_RESET ......................................................................
vboot: Remove hard dependency on MISSING_BOARD_RESET
Having a working board reset is certainly better when you're running vboot (because otherwise you'll hang when transitioning into recovery mode), but I don't think it should be strictly required, since it's still somewhat usable without. This is particularly important for certain test platforms that don't have a good way to reset but might still be useful for vboot testing/prototyping.
Change-Id: Ia765f54b6e2e176e2d54478fb1e0839d8cab9849 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/security/vboot/Kconfig 1 file changed, 0 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/38417/1
diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index 787cdbe..bde3980 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -28,7 +28,6 @@ default n select VBOOT_LIB select VBOOT_MOCK_SECDATA if !TPM1 && !TPM2 - depends on !MISSING_BOARD_RESET help Enabling VBOOT will use vboot to verify the components of the firmware (stages, payload, etc).
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38417 )
Change subject: vboot: Remove hard dependency on MISSING_BOARD_RESET ......................................................................
Patch Set 1: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38417 )
Change subject: vboot: Remove hard dependency on MISSING_BOARD_RESET ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/38417/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/38417/1/src/security/vboot/Kconfig@... PS1, Line 31: depends on !MISSING_BOARD_RESET As this is the last explicit dependency, what's left after it's removal are all the implicit dependencies of the various `config VBOOT` overrides across the tree. Either every board has to override it, or we need to keep an explicit dependency here, e.g.
depends on 0 = 0
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38417 )
Change subject: vboot: Remove hard dependency on MISSING_BOARD_RESET ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38417/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/38417/1/src/security/vboot/Kconfig@... PS1, Line 31: depends on !MISSING_BOARD_RESET
As this is the last explicit dependency, what's left after it's removal […]
Alternatively, clean up the tree, e.g. for the mainboards
config VBOOT_SELECTS def_bool VBOOT select ...
Hello Aaron Durbin, build bot (Jenkins), Nico Huber, Joel Kitching,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38417
to look at the new patch set (#2).
Change subject: vboot: Remove hard dependency on MISSING_BOARD_RESET ......................................................................
vboot: Remove hard dependency on MISSING_BOARD_RESET
Having a working board reset is certainly better when you're running vboot (because otherwise you'll hang when transitioning into recovery mode), but I don't think it should be strictly required, since it's still somewhat usable without. This is particularly important for certain test platforms that don't have a good way to reset but might still be useful for vboot testing/prototyping.
Change-Id: Ia765f54b6e2e176e2d54478fb1e0839d8cab9849 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/security/vboot/Kconfig 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/38417/2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38417 )
Change subject: vboot: Remove hard dependency on MISSING_BOARD_RESET ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38417/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/38417/1/src/security/vboot/Kconfig@... PS1, Line 31: depends on !MISSING_BOARD_RESET
Alternatively, clean up the tree, e.g. for the mainboards […]
I bow to your Kconfig mastery... I would have never figured that out. Thanks!
I agree rewriting all the boards may be a good idea but I don't want to touch all that right now.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38417 )
Change subject: vboot: Remove hard dependency on MISSING_BOARD_RESET ......................................................................
Patch Set 2: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38417 )
Change subject: vboot: Remove hard dependency on MISSING_BOARD_RESET ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/38417/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/38417/1/src/security/vboot/Kconfig@... PS1, Line 31: depends on !MISSING_BOARD_RESET
I bow to your Kconfig mastery... I would have never figured that out. Thanks! […]
/me was left speechless by Nico's vast knowledge of Kconfig.
Would it be a good idea to replace this with a MAINBOARD_HAS_VBOOT symbol? I don't think selecting this when the board has no vboot support makes much sense.
mturney mturney has uploaded a new patch set (#3) to the change originally created by Julius Werner. ( https://review.coreboot.org/c/coreboot/+/38417 )
Change subject: vboot: Remove hard dependency on MISSING_BOARD_RESET ......................................................................
vboot: Remove hard dependency on MISSING_BOARD_RESET
Having a working board reset is certainly better when you're running vboot (because otherwise you'll hang when transitioning into recovery mode), but I don't think it should be strictly required, since it's still somewhat usable without. This is particularly important for certain test platforms that don't have a good way to reset but might still be useful for vboot testing/prototyping.
Change-Id: Ia765f54b6e2e176e2d54478fb1e0839d8cab9849 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/security/vboot/Kconfig 1 file changed, 0 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/38417/3
Hello Aaron Durbin, Angel Pons, Ravi kumar, build bot (Jenkins), Nico Huber, Joel Kitching,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38417
to look at the new patch set (#5).
Change subject: vboot: Remove hard dependency on MISSING_BOARD_RESET ......................................................................
vboot: Remove hard dependency on MISSING_BOARD_RESET
Having a working board reset is certainly better when you're running vboot (because otherwise you'll hang when transitioning into recovery mode), but I don't think it should be strictly required, since it's still somewhat usable without. This is particularly important for certain test platforms that don't have a good way to reset but might still be useful for vboot testing/prototyping.
Change-Id: Ia765f54b6e2e176e2d54478fb1e0839d8cab9849 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/security/vboot/Kconfig 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/38417/5
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38417 )
Change subject: vboot: Remove hard dependency on MISSING_BOARD_RESET ......................................................................
Patch Set 5: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38417 )
Change subject: vboot: Remove hard dependency on MISSING_BOARD_RESET ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38417/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/38417/1/src/security/vboot/Kconfig@... PS1, Line 31: depends on !MISSING_BOARD_RESET
/me was left speechless by Nico's vast knowledge of Kconfig. […]
I don't think there is a MAINBOARD_HAS_VBOOT? At least I'm not seeing one. (There is a VBOOT_NO_BOARD_SUPPORT which mainboards that don't have an explicit recovery switch and things like that are supposed to select, but we sometimes still want to run vboot on those.)
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38417 )
Change subject: vboot: Remove hard dependency on MISSING_BOARD_RESET ......................................................................
vboot: Remove hard dependency on MISSING_BOARD_RESET
Having a working board reset is certainly better when you're running vboot (because otherwise you'll hang when transitioning into recovery mode), but I don't think it should be strictly required, since it's still somewhat usable without. This is particularly important for certain test platforms that don't have a good way to reset but might still be useful for vboot testing/prototyping.
Change-Id: Ia765f54b6e2e176e2d54478fb1e0839d8cab9849 Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/38417 Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/security/vboot/Kconfig 1 file changed, 1 insertion(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Aaron Durbin: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve
diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index 7e86c7c..30b99af 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -28,7 +28,7 @@ default n select VBOOT_LIB select VBOOT_MOCK_SECDATA if !TPM1 && !TPM2 - depends on !MISSING_BOARD_RESET + depends on 0 = 0 # Must have a 'depends on' or board overrides will break it. help Enabling VBOOT will use vboot to verify the components of the firmware (stages, payload, etc).
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38417 )
Change subject: vboot: Remove hard dependency on MISSING_BOARD_RESET ......................................................................
Patch Set 6:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/254 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/253 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/252
Please note: This test is under development and might not be accurate at all!
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38417 )
Change subject: vboot: Remove hard dependency on MISSING_BOARD_RESET ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38417/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/38417/1/src/security/vboot/Kconfig@... PS1, Line 31: depends on !MISSING_BOARD_RESET
I don't think there is a MAINBOARD_HAS_VBOOT? At least I'm not seeing one. […]
It would have to be created, sorry if I wasn't clear.
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
Angel Pons (Code Review)
-
Julius Werner (Code Review)
-
mturney mturney (Code Review)
-
Nico Huber (Code Review)