Change in coreboot[master]: util/amdfwtool: Clarify APOB NV requirements

[Marshall Dawson (Code Review)]

Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38690 )

Change subject: util/amdfwtool: Clarify APOB NV requirements
......................................................................

util/amdfwtool: Clarify APOB NV requirements

Relocate the first size check.  This was automatically continuing
and not looking for the caller incorrectly passing a destination.

New information indicates that the APOB_NV should always be present
in the system.  Augment the missing size check to inferring whether
a missing size is valid, as in the case of older products, or truly
missing when it's needed.

Signed-off-by: Marshall Dawson 
Change-Id: I51f5333de4392dec1478bd84563c053a508b9e9e
---
M util/amdfwtool/amdfwtool.c
1 file changed, 25 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/38690/1

diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c
index d5c63de..c41da58 100644
--- a/util/amdfwtool/amdfwtool.c
+++ b/util/amdfwtool/amdfwtool.c
@@ -808,6 +808,17 @@
    return 0;
 }

+static int find_bios_entry(amd_bios_type type)
+{
+	int i;
+
+	for (i = 0; amd_bios_table[i].type != AMD_BIOS_INVALID; i++) {
+		if (amd_bios_table[i].type == type)
+			return i;
+	}
+	return -1;
+}
+
 static void integrate_bios_firmwares(context *ctx,
    				bios_directory_table *biosdir,
    				bios_directory_table *biosdir2,
@@ -817,6 +828,7 @@
    ssize_t bytes;
    unsigned int i, count;
    int level;
+	int apob_idx;

/* This function can create a primary table, a secondary table, or a
     * flattened table which contains all applicable types.  These if-else
@@ -843,9 +855,6 @@
    			fw_table[i].type != AMD_BIOS_L2_PTR &&
    			fw_table[i].type != AMD_BIOS_BIN))
    		continue;
-		/* APOB_NV needs a size, else no S3 and skip item */
-		if (fw_table[i].type == AMD_BIOS_APOB_NV && !fw_table[i].size)
-			continue;

/* BIOS Directory items may have additional requirements */

@@ -857,6 +866,19 @@
    			exit(1);
    		}
    	}
+		/* APOB_NV needs a size, else no choice but to skip the item */
+		if (fw_table[i].type == AMD_BIOS_APOB_NV && !fw_table[i].size) {
+			/* Attempt to determine whether this is an error */
+			apob_idx = find_bios_entry(AMD_BIOS_APOB);
+			if (apob_idx < 0 || !fw_table[apob_idx].dest) {
+				/* APOV NV not expected to be used */
+				continue;
+			} else {
+				printf("Error: APOB NV must have a size\n");
+				free(ctx->rom);
+				exit(1);
+			}
+		}

/* APOB_DATA needs destination */
    	if (fw_table[i].type == AMD_BIOS_APOB && !fw_table[i].dest) {

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38690 )

Change subject: util/amdfwtool: Clarify APOB NV requirements
......................................................................

Patch Set 1: Code-Review+2

[Martin Roth (Code Review)]

Martin Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38690 )

Change subject: util/amdfwtool: Clarify APOB NV requirements
......................................................................

util/amdfwtool: Clarify APOB NV requirements

Relocate the first size check.  This was automatically continuing
and not looking for the caller incorrectly passing a destination.

New information indicates that the APOB_NV should always be present
in the system.  Augment the missing size check to inferring whether
a missing size is valid, as in the case of older products, or truly
missing when it's needed.

Signed-off-by: Marshall Dawson 
Change-Id: I51f5333de4392dec1478bd84563c053a508b9e9e
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38690
Tested-by: build bot (Jenkins) 
Reviewed-by: Martin Roth 
---
M util/amdfwtool/amdfwtool.c
1 file changed, 25 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Martin Roth: Looks good to me, approved

diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c
index 379caaa..5bcc0a7 100644
--- a/util/amdfwtool/amdfwtool.c
+++ b/util/amdfwtool/amdfwtool.c
@@ -808,6 +808,17 @@
    return 0;
 }

+static int find_bios_entry(amd_bios_type type)
+{
+	int i;
+
+	for (i = 0; amd_bios_table[i].type != AMD_BIOS_INVALID; i++) {
+		if (amd_bios_table[i].type == type)
+			return i;
+	}
+	return -1;
+}
+
 static void integrate_bios_firmwares(context *ctx,
    				bios_directory_table *biosdir,
    				bios_directory_table *biosdir2,
@@ -817,6 +828,7 @@
    ssize_t bytes;
    unsigned int i, count;
    int level;
+	int apob_idx;

/* This function can create a primary table, a secondary table, or a
     * flattened table which contains all applicable types.  These if-else
@@ -843,9 +855,6 @@
    			fw_table[i].type != AMD_BIOS_L2_PTR &&
    			fw_table[i].type != AMD_BIOS_BIN))
    		continue;
-		/* APOB_NV needs a size, else no S3 and skip item */
-		if (fw_table[i].type == AMD_BIOS_APOB_NV && !fw_table[i].size)
-			continue;

/* BIOS Directory items may have additional requirements */

@@ -857,6 +866,19 @@
    			exit(1);
    		}
    	}
+		/* APOB_NV needs a size, else no choice but to skip the item */
+		if (fw_table[i].type == AMD_BIOS_APOB_NV && !fw_table[i].size) {
+			/* Attempt to determine whether this is an error */
+			apob_idx = find_bios_entry(AMD_BIOS_APOB);
+			if (apob_idx < 0 || !fw_table[apob_idx].dest) {
+				/* APOV NV not expected to be used */
+				continue;
+			} else {
+				printf("Error: APOB NV must have a size\n");
+				free(ctx->rom);
+				exit(1);
+			}
+		}

/* APOB_DATA needs destination */
    	if (fw_table[i].type == AMD_BIOS_APOB && !fw_table[i].dest) {

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38690 )

Change subject: util/amdfwtool: Clarify APOB NV requirements
......................................................................

Patch Set 2:

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/1037
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1036
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1035

Please note: This test is under development and might not be accurate at all!