Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36210 )
Change subject: security/vboot: Add new Kconfig option VBOOT_EARLY_EC_SYNC ......................................................................
security/vboot: Add new Kconfig option VBOOT_EARLY_EC_SYNC
The new Kconfig option compiles EC software sync into romstage.
Change-Id: I56edcd385467edb5e91168dc59427d86fa9581d7 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc 2 files changed, 10 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/36210/1
diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index d6d74ca..7a2debe 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -224,6 +224,13 @@ Add a space delimited list of filenames that should only be in the RO section.
+config VBOOT_EARLY_EC_SYNC + bool "Perform EC software sync in romstage" + default n + help + When selected, this will perform software sync on the primary EC only + in romstage, before memory training runs. + menu "GBB configuration"
config GBB_HWID diff --git a/src/security/vboot/Makefile.inc b/src/security/vboot/Makefile.inc index 31c0f5d..a05fd7d 100644 --- a/src/security/vboot/Makefile.inc +++ b/src/security/vboot/Makefile.inc @@ -37,6 +37,8 @@ romstage-y += vbnv.c ramstage-y += vbnv.c
+romstage-$(CONFIG_VBOOT_EARLY_EC_SYNC) += sync_ec.c + bootblock-$(CONFIG_VBOOT_VBNV_CMOS) += vbnv_cmos.c verstage-$(CONFIG_VBOOT_VBNV_CMOS) += vbnv_cmos.c romstage-$(CONFIG_VBOOT_VBNV_CMOS) += vbnv_cmos.c @@ -130,6 +132,7 @@ endef # vboot-for-stage
CFLAGS_common += -I3rdparty/vboot/firmware/2lib/include +CFLAGS_common += -I3rdparty/vboot/firmware/lib/include
$(eval $(call vboot-for-stage,bootblock)) $(eval $(call vboot-for-stage,romstage))
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36210 )
Change subject: security/vboot: Add new Kconfig option VBOOT_EARLY_EC_SYNC ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36210/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36210/1/src/security/vboot/Kconfig@... PS1, Line 228: "Perform EC software sync in romstage" This option shows on all targets, even non chromeos ones. Please guard this properly.
Hello Aaron Durbin, Karthik Ramasubramanian, Julius Werner, Duncan Laurie, Shelley Chen, build bot (Jenkins), Joel Kitching, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36210
to look at the new patch set (#2).
Change subject: security/vboot: Add new Kconfig option VBOOT_EARLY_EC_SYNC ......................................................................
security/vboot: Add new Kconfig option VBOOT_EARLY_EC_SYNC
The new Kconfig option compiles EC software sync into romstage.
Change-Id: I56edcd385467edb5e91168dc59427d86fa9581d7 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc 2 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/36210/2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36210 )
Change subject: security/vboot: Add new Kconfig option VBOOT_EARLY_EC_SYNC ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36210/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36210/1/src/security/vboot/Kconfig@... PS1, Line 228: "Perform EC software sync in romstage"
This option shows on all targets, even non chromeos ones. Please guard this properly.
Ack
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36210 )
Change subject: security/vboot: Add new Kconfig option VBOOT_EARLY_EC_SYNC ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/36210/2/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36210/2/src/security/vboot/Kconfig@... PS2, Line 232: primary EC I think we want to stop referring to the EC as "primary" or "secondary" and just say EC. Going forward, there should only ever be one.
https://review.coreboot.org/c/coreboot/+/36210/2/src/security/vboot/Kconfig@... PS2, Line 232: When selected, this will perform Maybe just "Enables EC software sync..."
https://review.coreboot.org/c/coreboot/+/36210/2/src/security/vboot/Kconfig@... PS2, Line 233: . May want to add some more explanation as to why this option might be useful (i.e. perform USB-PD power negotiation earlier on). Also add any kind of drawbacks (may add some extra boot time? I'm not sure about this).
https://review.coreboot.org/c/coreboot/+/36210/2/src/security/vboot/Makefile... File src/security/vboot/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36210/2/src/security/vboot/Makefile... PS2, Line 40: sync_ec.c Might want to call this file ec_sync.c to be more consistent with vboot's naming...
https://review.coreboot.org/c/coreboot/+/36210/2/src/security/vboot/Makefile... PS2, Line 134: CFLAGS_common += -I3rdparty/vboot/firmware/2lib/include : CFLAGS_common += -I3rdparty/vboot/firmware/lib/include Rather than adding vboot1 dependencies in coreboot (which I already worked really hard to get rid of!), can we work to migrate firmware/lib/ec_sync.c to firmware/2lib/2ec_sync.c and firmware/2lib/2auxfw_sync.c as I mentioned?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36210 )
Change subject: security/vboot: Add new Kconfig option VBOOT_EARLY_EC_SYNC ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36210/2/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36210/2/src/security/vboot/Kconfig@... PS2, Line 232: primary EC
I think we want to stop referring to the EC as "primary" or "secondary" and just say EC. […]
Done
https://review.coreboot.org/c/coreboot/+/36210/2/src/security/vboot/Kconfig@... PS2, Line 232: When selected, this will perform
Maybe just "Enables EC software sync... […]
Done
https://review.coreboot.org/c/coreboot/+/36210/2/src/security/vboot/Kconfig@... PS2, Line 233: .
May want to add some more explanation as to why this option might be useful (i.e. […]
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36210 )
Change subject: security/vboot: Add new Kconfig option VBOOT_EARLY_EC_SYNC ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36210/2/src/security/vboot/Makefile... File src/security/vboot/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36210/2/src/security/vboot/Makefile... PS2, Line 40: sync_ec.c
Might want to call this file ec_sync.c to be more consistent with vboot's naming...
Done
https://review.coreboot.org/c/coreboot/+/36210/2/src/security/vboot/Makefile... PS2, Line 134: CFLAGS_common += -I3rdparty/vboot/firmware/2lib/include : CFLAGS_common += -I3rdparty/vboot/firmware/lib/include
Rather than adding vboot1 dependencies in coreboot (which I already worked really hard to get rid of […]
Sure. Sorry, I don't have all the historical context here.
Hello Aaron Durbin, Karthik Ramasubramanian, Julius Werner, Duncan Laurie, Shelley Chen, build bot (Jenkins), Joel Kitching, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36210
to look at the new patch set (#3).
Change subject: security/vboot: Add new Kconfig option VBOOT_EARLY_EC_SYNC ......................................................................
security/vboot: Add new Kconfig option VBOOT_EARLY_EC_SYNC
The new Kconfig option compiles EC software sync into romstage.
Change-Id: I56edcd385467edb5e91168dc59427d86fa9581d7 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/cpu/intel/car/romstage.c M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc 3 files changed, 19 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/36210/3
Hello Patrick Rudolph, Aaron Durbin, Karthik Ramasubramanian, Julius Werner, Duncan Laurie, Shelley Chen, build bot (Jenkins), Joel Kitching, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36210
to look at the new patch set (#4).
Change subject: security/vboot: Add new Kconfig option VBOOT_EARLY_EC_SYNC ......................................................................
security/vboot: Add new Kconfig option VBOOT_EARLY_EC_SYNC
The new Kconfig option compiles EC software sync into romstage.
Change-Id: I56edcd385467edb5e91168dc59427d86fa9581d7 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc 2 files changed, 14 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/36210/4
Hello Patrick Rudolph, Aaron Durbin, Karthik Ramasubramanian, Julius Werner, Duncan Laurie, Shelley Chen, build bot (Jenkins), Joel Kitching, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36210
to look at the new patch set (#5).
Change subject: security/vboot: Add new Kconfig option VBOOT_EARLY_EC_SYNC ......................................................................
security/vboot: Add new Kconfig option VBOOT_EARLY_EC_SYNC
The new Kconfig option compiles EC software sync into romstage.
Change-Id: I56edcd385467edb5e91168dc59427d86fa9581d7 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc 2 files changed, 14 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/36210/5
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36210 )
Change subject: security/vboot: Add new Kconfig option VBOOT_EARLY_EC_SYNC ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36210/8/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36210/8/src/security/vboot/Kconfig@... PS8, Line 228: "Perform EC software sync in romstage" Does this need to be a user-visible option?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36210 )
Change subject: security/vboot: Add new Kconfig option VBOOT_EARLY_EC_SYNC ......................................................................
Patch Set 8:
Why isn't this patch squashed w/ the introduction of the feature and compilation unit itself?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36210 )
Change subject: security/vboot: Add new Kconfig option VBOOT_EARLY_EC_SYNC ......................................................................
Patch Set 8:
(1 comment)
Patch Set 8:
Why isn't this patch squashed w/ the introduction of the feature and compilation unit itself?
I guess it could be. The code as is should be able to run in ramstage or romstage, so this is the one that compiles it into romstage.
https://review.coreboot.org/c/coreboot/+/36210/8/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36210/8/src/security/vboot/Kconfig@... PS8, Line 228: "Perform EC software sync in romstage"
Does this need to be a user-visible option?
Hmm, probably not. Something I'd expect we'd dump into config.${BOARD}
Tim Wawrzynczak has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/36210 )
Change subject: security/vboot: Add new Kconfig option VBOOT_EARLY_EC_SYNC ......................................................................
Abandoned
Squashed into 32607
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Furquan Shaikh (Code Review)
-
Joel Kitching (Code Review)
-
Tim Wawrzynczak (Code Review)