Hello Aaron Durbin, Frans Hendriks, Philipp Deppenwiese,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/32716
to review the following change.
Change subject: vboot: Turn vboot_logic_executed() into a static inline ......................................................................
vboot: Turn vboot_logic_executed() into a static inline
This patch moves vboot_logic_executed() (and its dependencies) into a header and turns it into a static inline function. The function is used to guard larger amounts of code in several places, so this should allow us to safe some more space through compile-time elimination (and also makes it easier to avoid undefined reference issues in some cases).
Change-Id: I193f608882cbfe07dc91ee90d02fafbd67a3c324 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c 2 files changed, 58 insertions(+), 54 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/32716/1
diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index b4fae19..2a0523d 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -16,6 +16,8 @@ #ifndef __VBOOT_MISC_H__ #define __VBOOT_MISC_H__
+#include <assert.h> +#include <arch/early_variables.h> #include <security/vboot/vboot_common.h>
struct vb2_context; @@ -66,13 +68,63 @@ void vboot_fill_handoff(void);
/* - * Source: security/vboot/vboot_loader.c - */ -int vboot_logic_executed(void); - -/* * Source: security/vboot/bootmode.c */ void vboot_save_recovery_reason_vbnv(void);
+/* + * The stage loading code is compiled and entered from multiple stages. The + * helper functions below attempt to provide more clarity on when certain + * code should be called. They are implemented inline for better compile-time + * code elimination. + */ + +static inline int verification_should_run(void) +{ + if (CONFIG(VBOOT_SEPARATE_VERSTAGE)) + return ENV_VERSTAGE; + else if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) + return ENV_ROMSTAGE; + else if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) + return ENV_BOOTBLOCK; + else + dead_code(); +} + +static inline int verstage_should_load(void) +{ + if (CONFIG(VBOOT_SEPARATE_VERSTAGE)) + return ENV_BOOTBLOCK; + else + return 0; +} + +/* Only for use by functions in this header, do not access directly! */ +extern int vboot_executed; + +static inline int vboot_logic_executed(void) +{ + /* If we are in the stage that runs verification, or in the stage that + both loads the verstage and is returned to from it afterwards, we + need to check a global to see if verfication has run. */ + if (verification_should_run() || + (verstage_should_load() && CONFIG(VBOOT_RETURN_FROM_VERSTAGE))) + return car_get_var(vboot_executed); + + if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) { + /* All other stages are "after the bootblock" */ + return !ENV_BOOTBLOCK; + } else if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) { + /* Post-RAM stages are "after the romstage" */ +#ifdef __PRE_RAM__ + return 0; +#else + return 1; +#endif + } else { + dead_code(); + } +} + + #endif /* __VBOOT_MISC_H__ */ diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index 3bbb3da..1350307 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -36,55 +36,7 @@ CONFIG(VBOOT_SEPARATE_VERSTAGE), "return from verstage only makes sense for separate verstages");
-/* The stage loading code is compiled and entered from multiple stages. The - * helper functions below attempt to provide more clarity on when certain - * code should be called. */ - -static int verification_should_run(void) -{ - if (CONFIG(VBOOT_SEPARATE_VERSTAGE)) - return ENV_VERSTAGE; - else if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) - return ENV_ROMSTAGE; - else if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) - return ENV_BOOTBLOCK; - else - die("impossible!"); -} - -static int verstage_should_load(void) -{ - if (CONFIG(VBOOT_SEPARATE_VERSTAGE)) - return ENV_BOOTBLOCK; - else - return 0; -} - -static int vboot_executed CAR_GLOBAL; - -int vboot_logic_executed(void) -{ - /* If we are in the stage that runs verification, or in the stage that - both loads the verstage and is returned to from it afterwards, we - need to check a global to see if verfication has run. */ - if (verification_should_run() || - (verstage_should_load() && CONFIG(VBOOT_RETURN_FROM_VERSTAGE))) - return car_get_var(vboot_executed); - - if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) { - /* All other stages are "after the bootblock" */ - return !ENV_BOOTBLOCK; - } else if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) { - /* Post-RAM stages are "after the romstage" */ -#ifdef __PRE_RAM__ - return 0; -#else - return 1; -#endif - } else { - die("impossible!"); - } -} +int vboot_executed CAR_GLOBAL;
static void vboot_prepare(void) {
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32716 )
Change subject: vboot: Turn vboot_logic_executed() into a static inline ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32716/1/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/#/c/32716/1/src/security/vboot/misc.h@90 PS1, Line 90: else else is not generally useful after a break or return
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32716 )
Change subject: vboot: Turn vboot_logic_executed() into a static inline ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
https://review.coreboot.org/#/c/32716/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32716/1//COMMIT_MSG@12 PS1, Line 12: safe save
https://review.coreboot.org/#/c/32716/1/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/#/c/32716/1/src/security/vboot/misc.h@103 PS1, Line 103: extern int vboot_executed; Can we move the extern into the inline function itself such that it would prevent people from accessing the global in correctly?
Hello Aaron Durbin, Frans Hendriks, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32716
to look at the new patch set (#2).
Change subject: vboot: Turn vboot_logic_executed() into a static inline ......................................................................
vboot: Turn vboot_logic_executed() into a static inline
This patch moves vboot_logic_executed() (and its dependencies) into a header and turns it into a static inline function. The function is used to guard larger amounts of code in several places, so this should allow us to save some more space through compile-time elimination (and also makes it easier to avoid undefined reference issues in some cases).
Change-Id: I193f608882cbfe07dc91ee90d02fafbd67a3c324 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c 2 files changed, 57 insertions(+), 54 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/32716/2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32716 )
Change subject: vboot: Turn vboot_logic_executed() into a static inline ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/32716/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32716/1//COMMIT_MSG@12 PS1, Line 12: safe
save
Done
https://review.coreboot.org/#/c/32716/1/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/#/c/32716/1/src/security/vboot/misc.h@103 PS1, Line 103: extern int vboot_executed;
Can we move the extern into the inline function itself such that it would prevent people from access […]
Done
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32716 )
Change subject: vboot: Turn vboot_logic_executed() into a static inline ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32716/2/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/#/c/32716/2/src/security/vboot/misc.h@90 PS2, Line 90: else else is not generally useful after a break or return
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32716 )
Change subject: vboot: Turn vboot_logic_executed() into a static inline ......................................................................
Patch Set 2: Code-Review+2
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32716 )
Change subject: vboot: Turn vboot_logic_executed() into a static inline ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/32716/2/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/#/c/32716/2/src/security/vboot/misc.h@90 PS2, Line 90: else
else is not generally useful after a break or return
Not sure about code style: Suggest removing the latest else, having dead_code() default return. Similar in verstage_should_load() and vboot_logic_executed()
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32716 )
Change subject: vboot: Turn vboot_logic_executed() into a static inline ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32716/2/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/#/c/32716/2/src/security/vboot/misc.h@90 PS2, Line 90: else
Not sure about code style: Suggest removing the latest else, having dead_code() default return. […]
I'd call this one a matter of taste (it's not in the code style to my knowledge). In cases where many statements follow in the else block I agree with not indenting them, but where every branch is just a single line and they all present equally weighed options, I think this may read better. Also I'm just moving code around that was already formatted this way.
Julius Werner has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32716 )
Change subject: vboot: Turn vboot_logic_executed() into a static inline ......................................................................
vboot: Turn vboot_logic_executed() into a static inline
This patch moves vboot_logic_executed() (and its dependencies) into a header and turns it into a static inline function. The function is used to guard larger amounts of code in several places, so this should allow us to save some more space through compile-time elimination (and also makes it easier to avoid undefined reference issues in some cases).
Change-Id: I193f608882cbfe07dc91ee90d02fafbd67a3c324 Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/32716 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Frans Hendriks fhendriks@eltan.com --- M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c 2 files changed, 57 insertions(+), 54 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Frans Hendriks: Looks good to me, but someone else must approve
diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index b4fae19..23159c8 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -16,6 +16,8 @@ #ifndef __VBOOT_MISC_H__ #define __VBOOT_MISC_H__
+#include <assert.h> +#include <arch/early_variables.h> #include <security/vboot/vboot_common.h>
struct vb2_context; @@ -66,13 +68,62 @@ void vboot_fill_handoff(void);
/* - * Source: security/vboot/vboot_loader.c - */ -int vboot_logic_executed(void); - -/* * Source: security/vboot/bootmode.c */ void vboot_save_recovery_reason_vbnv(void);
+/* + * The stage loading code is compiled and entered from multiple stages. The + * helper functions below attempt to provide more clarity on when certain + * code should be called. They are implemented inline for better compile-time + * code elimination. + */ + +static inline int verification_should_run(void) +{ + if (CONFIG(VBOOT_SEPARATE_VERSTAGE)) + return ENV_VERSTAGE; + else if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) + return ENV_ROMSTAGE; + else if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) + return ENV_BOOTBLOCK; + else + dead_code(); +} + +static inline int verstage_should_load(void) +{ + if (CONFIG(VBOOT_SEPARATE_VERSTAGE)) + return ENV_BOOTBLOCK; + else + return 0; +} + +static inline int vboot_logic_executed(void) +{ + extern int vboot_executed; /* should not be globally accessible */ + + /* If we are in the stage that runs verification, or in the stage that + both loads the verstage and is returned to from it afterwards, we + need to check a global to see if verfication has run. */ + if (verification_should_run() || + (verstage_should_load() && CONFIG(VBOOT_RETURN_FROM_VERSTAGE))) + return car_get_var(vboot_executed); + + if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) { + /* All other stages are "after the bootblock" */ + return !ENV_BOOTBLOCK; + } else if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) { + /* Post-RAM stages are "after the romstage" */ +#ifdef __PRE_RAM__ + return 0; +#else + return 1; +#endif + } else { + dead_code(); + } +} + + #endif /* __VBOOT_MISC_H__ */ diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index 3bbb3da..1350307 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -36,55 +36,7 @@ CONFIG(VBOOT_SEPARATE_VERSTAGE), "return from verstage only makes sense for separate verstages");
-/* The stage loading code is compiled and entered from multiple stages. The - * helper functions below attempt to provide more clarity on when certain - * code should be called. */ - -static int verification_should_run(void) -{ - if (CONFIG(VBOOT_SEPARATE_VERSTAGE)) - return ENV_VERSTAGE; - else if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) - return ENV_ROMSTAGE; - else if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) - return ENV_BOOTBLOCK; - else - die("impossible!"); -} - -static int verstage_should_load(void) -{ - if (CONFIG(VBOOT_SEPARATE_VERSTAGE)) - return ENV_BOOTBLOCK; - else - return 0; -} - -static int vboot_executed CAR_GLOBAL; - -int vboot_logic_executed(void) -{ - /* If we are in the stage that runs verification, or in the stage that - both loads the verstage and is returned to from it afterwards, we - need to check a global to see if verfication has run. */ - if (verification_should_run() || - (verstage_should_load() && CONFIG(VBOOT_RETURN_FROM_VERSTAGE))) - return car_get_var(vboot_executed); - - if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) { - /* All other stages are "after the bootblock" */ - return !ENV_BOOTBLOCK; - } else if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) { - /* Post-RAM stages are "after the romstage" */ -#ifdef __PRE_RAM__ - return 0; -#else - return 1; -#endif - } else { - die("impossible!"); - } -} +int vboot_executed CAR_GLOBAL;
static void vboot_prepare(void) {
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32716 )
Change subject: vboot: Turn vboot_logic_executed() into a static inline ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32716/3/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/#/c/32716/3/src/security/vboot/misc.h@90 PS3, Line 90: else else is not generally useful after a break or return
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32716 )
Change subject: vboot: Turn vboot_logic_executed() into a static inline ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32716/4/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/32716/4/src/security/vboot/misc.h@1... PS4, Line 118: __PRE_RAM__ I guess we should replace this with 'if (ENV_ROMSTAGE_OR_BEFORE)'?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32716 )
Change subject: vboot: Turn vboot_logic_executed() into a static inline ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32716/4/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/32716/4/src/security/vboot/misc.h@1... PS4, Line 118: __PRE_RAM__
I guess we should replace this with 'if (ENV_ROMSTAGE_OR_BEFORE)'?
Already done? CB:35387
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32716 )
Change subject: vboot: Turn vboot_logic_executed() into a static inline ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32716/4/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/32716/4/src/security/vboot/misc.h@1... PS4, Line 118: __PRE_RAM__
Already done? CB:35387
I thought this patch was fresher than it was when following links. Sorry for the noise.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32716 )
Change subject: vboot: Turn vboot_logic_executed() into a static inline ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32716/4/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/32716/4/src/security/vboot/misc.h@1... PS4, Line 104: extern int vboot_executed; -Wnested-externs won't like it
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32716 )
Change subject: vboot: Turn vboot_logic_executed() into a static inline ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32716/4/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/32716/4/src/security/vboot/misc.h@1... PS4, Line 104: extern int vboot_executed;
-Wnested-externs won't like it
Do... we not like nested externs? I mean, all that warning does is point out constructs like this -- it doesn't check for mistakes or anything, it's just a style enforcer. I think this notation has good, valid uses (e.g. it allows this function to be a static inline while still preventing other code from accessing a global directly which should only be accessed through this helper function). I don't see why we should make the compiler ban it.
-
Aaron Durbin (Code Review) -
build bot (Jenkins) (Code Review)
-
Frans Hendriks (Code Review)
-
HAOUAS Elyes (Code Review)
-
Julius Werner (Code Review)