Frans Hendriks has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33442
Change subject: vendorcode/eltan/security: Use config VENDORCODE_ELTAN_XXX ......................................................................
vendorcode/eltan/security: Use config VENDORCODE_ELTAN_XXX
To avoid confusion use VENDORCODE_ELTAN_VBOOT and VENDORCODE_ELTAN_MBOOT config values.
Inlcude verfied_boot and mboot subdirectories as CPPFLAGS when measured boot or verified boot is enabled. This allows to generate binary with measured boot enabled only.
BUG=N/A TEST=Boot Linux 4.20 and verify logging on Facebook FBG-1701
Change-Id: Iaaf3c8cacbc8d2be7387264ca9c973e583871f0a Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/vendorcode/eltan/security/Makefile.inc 1 file changed, 2 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/33442/1
diff --git a/src/vendorcode/eltan/security/Makefile.inc b/src/vendorcode/eltan/security/Makefile.inc index 26b324b..16f17fd 100644 --- a/src/vendorcode/eltan/security/Makefile.inc +++ b/src/vendorcode/eltan/security/Makefile.inc @@ -1,6 +1,6 @@ ## This file is part of the coreboot project. ## -## Copyright (C) 2018 Eltan B.V. +## Copyright (C) 2018-2019 Eltan B.V. ## ## This program is free software; you can redistribute it and/or modify ## it under the terms of the GNU General Public License as published by @@ -16,12 +16,9 @@ subdirs-y += verified_boot subdirs-y += mboot
-ifeq ($(CONFIG_MBOOT), y) +ifneq ($(filter y,$(CONFIG_VENDORCODE_ELTAN_VBOOT) $(CONFIG_VENDORCODE_ELTAN_MBOOT)),) CPPFLAGS_common += -I$(src)/vendorcode/eltan/security/mboot CPPFLAGS_common += -I$(src)/vendorcode/eltan/security/include -endif - -ifeq ($(CONFIG_VERIFIED_BOOT), y) CPPFLAGS_common += -I$(src)/vendorcode/eltan/security/verified_boot endif
Hello Patrick Rudolph, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33442
to look at the new patch set (#2).
Change subject: vendorcode/eltan/security: Use config VENDORCODE_ELTAN_XXX ......................................................................
vendorcode/eltan/security: Use config VENDORCODE_ELTAN_XXX
To avoid confusion use VENDORCODE_ELTAN_VBOOT and VENDORCODE_ELTAN_MBOOT config values.
Include verfied_boot and mboot subdirectories as CPPFLAGS when measured boot or verified boot is enabled. This allows to generate binary with measured boot enabled only.
BUG=N/A TEST=Boot Linux 4.20 and verify logging on Facebook FBG-1701
Change-Id: Iaaf3c8cacbc8d2be7387264ca9c973e583871f0a Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/vendorcode/eltan/security/Makefile.inc 1 file changed, 2 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/33442/2
Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33442 )
Change subject: vendorcode/eltan/security: Use config VENDORCODE_ELTAN_XXX ......................................................................
Patch Set 2: Code-Review+2
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33442 )
Change subject: vendorcode/eltan/security: Use config VENDORCODE_ELTAN_XXX ......................................................................
Patch Set 2: Code-Review-1
(1 comment)
-1 to get attention for my comment since it's at +2 now.
https://review.coreboot.org/c/coreboot/+/33442/2/src/vendorcode/eltan/securi... File src/vendorcode/eltan/security/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33442/2/src/vendorcode/eltan/securi... PS2, Line 19: $(CONFIG_VENDORCODE_ELTAN_VBOOT) $(CONFIG_VENDORCODE_ELTAN_MBOOT) So why not just have one config option? Now if either of these is set, it will do both.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33442 )
Change subject: vendorcode/eltan/security: Use config VENDORCODE_ELTAN_XXX ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33442/2/src/vendorcode/eltan/securi... File src/vendorcode/eltan/security/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33442/2/src/vendorcode/eltan/securi... PS2, Line 19: $(CONFIG_VENDORCODE_ELTAN_VBOOT) $(CONFIG_VENDORCODE_ELTAN_MBOOT)
So why not just have one config option? Now if either of these is set, it will do both.
Measured boot can be enabled independ of verified boot. Why to create a new config which indicates that MBOOT and/or VBOOT is enabled.?
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33442 )
Change subject: vendorcode/eltan/security: Use config VENDORCODE_ELTAN_XXX ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/33442/2/src/vendorcode/eltan/securi... File src/vendorcode/eltan/security/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33442/2/src/vendorcode/eltan/securi... PS2, Line 19: $(CONFIG_VENDORCODE_ELTAN_VBOOT) $(CONFIG_VENDORCODE_ELTAN_MBOOT)
Measured boot can be enabled independ of verified boot. […]
Done
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33442 )
Change subject: vendorcode/eltan/security: Use config VENDORCODE_ELTAN_XXX ......................................................................
vendorcode/eltan/security: Use config VENDORCODE_ELTAN_XXX
To avoid confusion use VENDORCODE_ELTAN_VBOOT and VENDORCODE_ELTAN_MBOOT config values.
Include verfied_boot and mboot subdirectories as CPPFLAGS when measured boot or verified boot is enabled. This allows to generate binary with measured boot enabled only.
BUG=N/A TEST=Boot Linux 4.20 and verify logging on Facebook FBG-1701
Change-Id: Iaaf3c8cacbc8d2be7387264ca9c973e583871f0a Signed-off-by: Frans Hendriks fhendriks@eltan.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33442 Reviewed-by: Lance Zhao lance.zhao@gmail.com Reviewed-by: Martin Roth martinroth@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/vendorcode/eltan/security/Makefile.inc 1 file changed, 2 insertions(+), 5 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved Lance Zhao: Looks good to me, approved
diff --git a/src/vendorcode/eltan/security/Makefile.inc b/src/vendorcode/eltan/security/Makefile.inc index 26b324b..16f17fd 100644 --- a/src/vendorcode/eltan/security/Makefile.inc +++ b/src/vendorcode/eltan/security/Makefile.inc @@ -1,6 +1,6 @@ ## This file is part of the coreboot project. ## -## Copyright (C) 2018 Eltan B.V. +## Copyright (C) 2018-2019 Eltan B.V. ## ## This program is free software; you can redistribute it and/or modify ## it under the terms of the GNU General Public License as published by @@ -16,12 +16,9 @@ subdirs-y += verified_boot subdirs-y += mboot
-ifeq ($(CONFIG_MBOOT), y) +ifneq ($(filter y,$(CONFIG_VENDORCODE_ELTAN_VBOOT) $(CONFIG_VENDORCODE_ELTAN_MBOOT)),) CPPFLAGS_common += -I$(src)/vendorcode/eltan/security/mboot CPPFLAGS_common += -I$(src)/vendorcode/eltan/security/include -endif - -ifeq ($(CONFIG_VERIFIED_BOOT), y) CPPFLAGS_common += -I$(src)/vendorcode/eltan/security/verified_boot endif
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33442 )
Change subject: vendorcode/eltan/security: Use config VENDORCODE_ELTAN_XXX ......................................................................
Patch Set 3:
(3 comments)
What's the complete picture here? I see Kconfig variables that are not defined and directories that do not exist?
https://review.coreboot.org/c/coreboot/+/33442/3/src/vendorcode/eltan/securi... File src/vendorcode/eltan/security/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33442/3/src/vendorcode/eltan/securi... PS3, Line 17: subdirs-y += mboot No such directory? (last two)
https://review.coreboot.org/c/coreboot/+/33442/3/src/vendorcode/eltan/securi... PS3, Line 19: ifneq ($(filter y,$(CONFIG_VENDORCODE_ELTAN_VBOOT) $(CONFIG_VENDORCODE_ELTAN_MBOOT)),) No such Kconfig defined?
https://review.coreboot.org/c/coreboot/+/33442/3/src/vendorcode/eltan/securi... PS3, Line 26: CPPFLAGS_common += -I$(src)/security/include No such directory?
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33442 )
Change subject: vendorcode/eltan/security: Use config VENDORCODE_ELTAN_XXX ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/33442/3/src/vendorcode/eltan/securi... File src/vendorcode/eltan/security/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33442/3/src/vendorcode/eltan/securi... PS3, Line 17: subdirs-y += mboot
No such directory? (last two)
Is in other patch set
https://review.coreboot.org/c/coreboot/+/33442/3/src/vendorcode/eltan/securi... PS3, Line 19: ifneq ($(filter y,$(CONFIG_VENDORCODE_ELTAN_VBOOT) $(CONFIG_VENDORCODE_ELTAN_MBOOT)),)
No such Kconfig defined?
Is in other patch set
https://review.coreboot.org/c/coreboot/+/33442/3/src/vendorcode/eltan/securi... PS3, Line 26: CPPFLAGS_common += -I$(src)/security/include
No such directory?
Is in other patch set
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33442 )
Change subject: vendorcode/eltan/security: Use config VENDORCODE_ELTAN_XXX ......................................................................
Patch Set 3:
Thanks.. So this was just merged out-of-order and the work that was supposed to go in first has not been merged for 6 weeks.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33442 )
Change subject: vendorcode/eltan/security: Use config VENDORCODE_ELTAN_XXX ......................................................................
Patch Set 3:
Patch Set 3:
Thanks.. So this was just merged out-of-order and the work that was supposed to go in first has not been merged for 6 weeks.
Other patches are still waiting for review (have +1 already ....