Change in coreboot[master]: eltan/security: Remove some preprocessor guards

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36632 )

Change subject: eltan/security: Remove some preprocessor guards
......................................................................

eltan/security: Remove some preprocessor guards

We generally let garbage-collection take care of unused functions.
While at it, move some related variable declarations in to the
header file and declare them const like they should be.

Change-Id: I7c6fa15bd45f861f13b6123ccb14c55415e42bc7
Signed-off-by: Kyösti Mälkki 
---
M src/vendorcode/eltan/security/mboot/mboot.c
M src/vendorcode/eltan/security/verified_boot/vboot_check.c
M src/vendorcode/eltan/security/verified_boot/vboot_check.h
3 files changed, 6 insertions(+), 22 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/36632/1

diff --git a/src/vendorcode/eltan/security/mboot/mboot.c b/src/vendorcode/eltan/security/mboot/mboot.c
index 4823c6a..228d1a0 100644
--- a/src/vendorcode/eltan/security/mboot/mboot.c
+++ b/src/vendorcode/eltan/security/mboot/mboot.c
@@ -266,7 +266,6 @@
    return status;
 }

-#ifdef __PRE_RAM__
 /*
  * Called from early romstage
  *
@@ -473,4 +472,3 @@

return status;
 }
-#endif // __PRE_RAM__
diff --git a/src/vendorcode/eltan/security/verified_boot/vboot_check.c b/src/vendorcode/eltan/security/verified_boot/vboot_check.c
index 88519bd..0e96f52 100644
--- a/src/vendorcode/eltan/security/verified_boot/vboot_check.c
+++ b/src/vendorcode/eltan/security/verified_boot/vboot_check.c
@@ -276,13 +276,11 @@
    	i++;
    }
 }
-#ifdef __BOOTBLOCK__
+
 /*
  * BOOTBLOCK
  */

-extern verify_item_t bootblock_verify_list[];
-
 void verified_boot_bootblock_check(void)
 {
    printk(BIOS_SPEW, "%s: processing bootblock items\n", __func__);
@@ -301,9 +299,7 @@
    printk(BIOS_SPEW, "%s: bootblock\n", __func__);
    verified_boot_bootblock_check();
 }
-#endif //__BOOTBLOCK__

-#ifdef __ROMSTAGE__
 /*
  * ROMSTAGE
  */
@@ -340,23 +336,17 @@
    	prepare_romstage = 1;
    }
 }
-#endif //__ROMSTAGE__

-#ifdef __POSTCAR__
 /*
  * POSTCAR
  */

-extern verify_item_t postcar_verify_list[];
-
 static void vendor_secure_prepare(void)
 {
    printk(BIOS_SPEW, "%s: postcar\n", __func__);
    process_verify_list(postcar_verify_list);
 }
-#endif //__POSTCAR__

-#ifdef __RAMSTAGE__
 /*
  * RAM STAGE
  */
@@ -408,10 +398,6 @@
    return 0;
 }

-extern verify_item_t payload_verify_list[];
-
-extern verify_item_t oprom_verify_list[];
-
 int verified_boot_should_run_oprom(struct rom_header *rom_header)
 {
    return process_oprom_list(oprom_verify_list, rom_header);
@@ -422,7 +408,6 @@
    printk(BIOS_SPEW, "%s: ramstage\n", __func__);
    process_verify_list(payload_verify_list);
 }
-#endif //__RAMSTAGE__

const struct cbfs_locator cbfs_master_header_locator = {
    .name = "Vendorcode Header Locator",
diff --git a/src/vendorcode/eltan/security/verified_boot/vboot_check.h b/src/vendorcode/eltan/security/verified_boot/vboot_check.h
index 22f1edf..36c8ffa 100644
--- a/src/vendorcode/eltan/security/verified_boot/vboot_check.h
+++ b/src/vendorcode/eltan/security/verified_boot/vboot_check.h
@@ -32,12 +32,8 @@
 /* These method verifies the SHA256 hash over the 'named' CBFS component.
  * 'type' denotes the type of CBFS component i.e. stage, payload or fsp.
  */
-#ifdef __BOOTBLOCK__
 void verified_boot_bootblock_check(void);
-#endif
-#ifdef __ROMSTAGE__
 void verified_boot_early_check(void);
-#endif

int verified_boot_check_manifest(void);

@@ -75,4 +71,9 @@

void process_verify_list(const verify_item_t list[]);

+extern const verify_item_t bootblock_verify_list[];
+extern const verify_item_t postcar_verify_list[];
+extern const verify_item_t payload_verify_list[];
+extern const verify_item_t oprom_verify_list[];
+
 #endif //VBOOT_CHECK_H

[Wim Vervoorn (Code Review)]

Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36632 )

Change subject: eltan/security: Remove some preprocessor guards
......................................................................

Patch Set 2: Code-Review+1

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36632 )

Change subject: eltan/security: Remove some preprocessor guards
......................................................................

Patch Set 2: Code-Review-1

This results in a build error: redefinition of vendor_secure_prepare()

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36632 )

Change subject: eltan/security: Remove some preprocessor guards
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36632/2/src/vendorcode/eltan/securi... 
File src/vendorcode/eltan/security/verified_boot/vboot_check.c:

https://review.coreboot.org/c/coreboot/+/36632/2/src/vendorcode/eltan/securi... 
PS2, Line 406: static void vendor_secure_prepare(void)
If you remove the other vendor_secure_prepare() and change this function into:

static void vendor_secure_prepare(void)
{
    if (ENV_BOOTBLOCK) {
    	printk(BIOS_SPEW, "%s: bootblock\n", __func__);
    	verified_boot_bootblock_check();
    }

if (ENV_ROMSTAGE) {
    	printk(BIOS_SPEW, "%s: romstage\n", __func__);
    	if (!prepare_romstage) {
    		verified_boot_early_check();
    		prepare_romstage = 1;
    	}
    }

if (ENV_RAMSTAGE) {
    	printk(BIOS_SPEW, "%s: ramstage\n", __func__);
    	process_verify_list(payload_verify_list);
    }
    
    if (ENV_POSTCAR) {
    	printk(BIOS_SPEW, "%s: postcar\n", __func__);
    	process_verify_list(postcar_verify_list);
    }
}

it will work

[Kyösti Mälkki (Code Review)]

Hello Wim Vervoorn, Frans Hendriks, wvervoorn, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/36632

to look at the new patch set (#3).

Change subject: eltan/security: Remove some preprocessor guards
......................................................................

eltan/security: Remove some preprocessor guards

We generally let garbage-collection take care of unused functions.
While at it, move some related variable declarations in to the
header file and declare them const like they should be.

Change-Id: I7c6fa15bd45f861f13b6123ccb14c55415e42bc7
Signed-off-by: Kyösti Mälkki 
---
M src/vendorcode/eltan/security/mboot/mboot.c
M src/vendorcode/eltan/security/verified_boot/vboot_check.c
M src/vendorcode/eltan/security/verified_boot/vboot_check.h
3 files changed, 29 insertions(+), 51 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/36632/3

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36632 )

Change subject: eltan/security: Remove some preprocessor guards
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36632/2/src/vendorcode/eltan/securi... 
File src/vendorcode/eltan/security/verified_boot/vboot_check.c:

https://review.coreboot.org/c/coreboot/+/36632/2/src/vendorcode/eltan/securi... 
PS2, Line 406: static void vendor_secure_prepare(void)
> If you remove the other vendor_secure_prepare() and change this function into: […]

Hopefully I got this right now. Changed the order though and put postcar before ramstage.

If the support is completely upstreamed, you might want to add some sample under configs/ such that this implementation sees automatic build-testing.

[Wim Vervoorn (Code Review)]

Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36632 )

Change subject: eltan/security: Remove some preprocessor guards
......................................................................

Patch Set 3: Code-Review+1

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36632 )

Change subject: eltan/security: Remove some preprocessor guards
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36632/2/src/vendorcode/eltan/securi... 
File src/vendorcode/eltan/security/verified_boot/vboot_check.c:

https://review.coreboot.org/c/coreboot/+/36632/2/src/vendorcode/eltan/securi... 
PS2, Line 406: static void vendor_secure_prepare(void)
> Hopefully I got this right now. Changed the order though and put postcar before ramstage. […]

I start working on this already: Update CB:34343

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36632 )

Change subject: eltan/security: Remove some preprocessor guards
......................................................................

Patch Set 3: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36632 )

Change subject: eltan/security: Remove some preprocessor guards
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36632/2/src/vendorcode/eltan/securi... 
File src/vendorcode/eltan/security/verified_boot/vboot_check.c:

https://review.coreboot.org/c/coreboot/+/36632/2/src/vendorcode/eltan/securi... 
PS2, Line 406: static void vendor_secure_prepare(void)
> I start working on this already: Update CB:34343

Done

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36632 )

Change subject: eltan/security: Remove some preprocessor guards
......................................................................

eltan/security: Remove some preprocessor guards

We generally let garbage-collection take care of unused functions.
While at it, move some related variable declarations in to the
header file and declare them const like they should be.

Change-Id: I7c6fa15bd45f861f13b6123ccb14c55415e42bc7
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/36632
Reviewed-by: Wim Vervoorn 
Reviewed-by: Patrick Georgi 
Tested-by: build bot (Jenkins) 
---
M src/vendorcode/eltan/security/mboot/mboot.c
M src/vendorcode/eltan/security/verified_boot/vboot_check.c
M src/vendorcode/eltan/security/verified_boot/vboot_check.h
3 files changed, 29 insertions(+), 51 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved
  Wim Vervoorn: Looks good to me, but someone else must approve

diff --git a/src/vendorcode/eltan/security/mboot/mboot.c b/src/vendorcode/eltan/security/mboot/mboot.c
index 4823c6a..228d1a0 100644
--- a/src/vendorcode/eltan/security/mboot/mboot.c
+++ b/src/vendorcode/eltan/security/mboot/mboot.c
@@ -266,7 +266,6 @@
    return status;
 }

-#ifdef __PRE_RAM__
 /*
  * Called from early romstage
  *
@@ -473,4 +472,3 @@

return status;
 }
-#endif // __PRE_RAM__
diff --git a/src/vendorcode/eltan/security/verified_boot/vboot_check.c b/src/vendorcode/eltan/security/verified_boot/vboot_check.c
index 88519bd..fdae7b8 100644
--- a/src/vendorcode/eltan/security/verified_boot/vboot_check.c
+++ b/src/vendorcode/eltan/security/verified_boot/vboot_check.c
@@ -276,13 +276,11 @@
    	i++;
    }
 }
-#ifdef __BOOTBLOCK__
+
 /*
  * BOOTBLOCK
  */

-extern verify_item_t bootblock_verify_list[];
-
 void verified_boot_bootblock_check(void)
 {
    printk(BIOS_SPEW, "%s: processing bootblock items\n", __func__);
@@ -296,14 +294,6 @@
    process_verify_list(bootblock_verify_list);
 }

-static void vendor_secure_prepare(void)
-{
-	printk(BIOS_SPEW, "%s: bootblock\n", __func__);
-	verified_boot_bootblock_check();
-}
-#endif //__BOOTBLOCK__
-
-#ifdef __ROMSTAGE__
 /*
  * ROMSTAGE
  */
@@ -330,33 +320,6 @@
    process_verify_list(romstage_verify_list);
 }

-static int prepare_romstage = 0;
-
-static void vendor_secure_prepare(void)
-{
-	printk(BIOS_SPEW, "%s: romstage\n", __func__);
-	if (!prepare_romstage) {
-		verified_boot_early_check();
-		prepare_romstage = 1;
-	}
-}
-#endif //__ROMSTAGE__
-
-#ifdef __POSTCAR__
-/*
- * POSTCAR
- */
-
-extern verify_item_t postcar_verify_list[];
-
-static void vendor_secure_prepare(void)
-{
-	printk(BIOS_SPEW, "%s: postcar\n", __func__);
-	process_verify_list(postcar_verify_list);
-}
-#endif //__POSTCAR__
-
-#ifdef __RAMSTAGE__
 /*
  * RAM STAGE
  */
@@ -408,10 +371,6 @@
    return 0;
 }

-extern verify_item_t payload_verify_list[];
-
-extern verify_item_t oprom_verify_list[];
-
 int verified_boot_should_run_oprom(struct rom_header *rom_header)
 {
    return process_oprom_list(oprom_verify_list, rom_header);
@@ -419,10 +378,30 @@

static void vendor_secure_prepare(void)
 {
-	printk(BIOS_SPEW, "%s: ramstage\n", __func__);
-	process_verify_list(payload_verify_list);
+	if (ENV_BOOTBLOCK) {
+		printk(BIOS_SPEW, "%s: bootblock\n", __func__);
+		verified_boot_bootblock_check();
+	}
+
+	if (ENV_ROMSTAGE) {
+		static int prepare_romstage = 0;
+		printk(BIOS_SPEW, "%s: romstage\n", __func__);
+		if (!prepare_romstage) {
+			verified_boot_early_check();
+			prepare_romstage = 1;
+		}
+	}
+
+	if (ENV_POSTCAR) {
+		printk(BIOS_SPEW, "%s: postcar\n", __func__);
+		process_verify_list(postcar_verify_list);
+	}
+
+	if (ENV_RAMSTAGE) {
+		printk(BIOS_SPEW, "%s: ramstage\n", __func__);
+		process_verify_list(payload_verify_list);
+	}
 }
-#endif //__RAMSTAGE__

const struct cbfs_locator cbfs_master_header_locator = {
    .name = "Vendorcode Header Locator",
diff --git a/src/vendorcode/eltan/security/verified_boot/vboot_check.h b/src/vendorcode/eltan/security/verified_boot/vboot_check.h
index 22f1edf..36c8ffa 100644
--- a/src/vendorcode/eltan/security/verified_boot/vboot_check.h
+++ b/src/vendorcode/eltan/security/verified_boot/vboot_check.h
@@ -32,12 +32,8 @@
 /* These method verifies the SHA256 hash over the 'named' CBFS component.
  * 'type' denotes the type of CBFS component i.e. stage, payload or fsp.
  */
-#ifdef __BOOTBLOCK__
 void verified_boot_bootblock_check(void);
-#endif
-#ifdef __ROMSTAGE__
 void verified_boot_early_check(void);
-#endif

int verified_boot_check_manifest(void);

@@ -75,4 +71,9 @@

void process_verify_list(const verify_item_t list[]);

+extern const verify_item_t bootblock_verify_list[];
+extern const verify_item_t postcar_verify_list[];
+extern const verify_item_t payload_verify_list[];
+extern const verify_item_t oprom_verify_list[];
+
 #endif //VBOOT_CHECK_H