Change in ...coreboot[master]: util/amdfwtool: Fix iteration of PSP firmwares

[Marshall Dawson (Code Review)]

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

Change subject: util/amdfwtool: Fix iteration of PSP firmwares
......................................................................

util/amdfwtool: Fix iteration of PSP firmwares

Correct an oversight in the utility that attempts to match up eligible
PSP directory table entries with blob names passed on the command
line.  A 1:1 matchup of items shouldn't be assumed, so the i iterator
shouldn't be used to walk both lists.

This change has no effect on google/grunt (Family 15h Models 70h-7Fh),
but eliminates blank entries of all FF's on builds of amd/bettong
(F15h 60h-6Fh) and pcengines/apu2 (F16h 30h-3Fh).  Removal of entries
also affects the checksum accordingly.

TEST=Build before/after images for grunt, bettong, apu2, and diff
     hexdumps of the amdfw.rom files

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/31726/1

diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c
index 406ae05..234760d 100644
--- a/util/amdfwtool/amdfwtool.c
+++ b/util/amdfwtool/amdfwtool.c
@@ -355,17 +355,18 @@
    int fd;
    ssize_t bytes;
    struct stat fd_stat;
-	unsigned int i;
+	unsigned int i, count;
    uint32_t rom_base_address = 0xFFFFFFFF - rom_size + 1;

-	for (i = 0; fw_table[i].type != AMD_FW_INVALID; i++) {
+	for (i = 0, count = 0; fw_table[i].type != AMD_FW_INVALID; i++) {
    	if (fw_table[i].type == AMD_PSP_FUSE_CHAIN) {
-			pspdir[4+4*i+0] = fw_table[i].type;
-			pspdir[4+4*i+1] = 0xFFFFFFFF;
-			pspdir[4+4*i+2] = 1;
-			pspdir[4+4*i+3] = 0;
+			pspdir[4+4*count+0] = fw_table[i].type;
+			pspdir[4+4*count+1] = 0xFFFFFFFF;
+			pspdir[4+4*count+2] = 1;
+			pspdir[4+4*count+3] = 0;
+			count++;
    	} else if (fw_table[i].filename != NULL) {
-			pspdir[4+4*i+0] = fw_table[i].type;
+			pspdir[4+4*count+0] = fw_table[i].type;

fd = open(fw_table[i].filename, O_RDONLY);
    		if (fd < 0) {
@@ -378,10 +379,10 @@
    			free(base);
    			exit(1);
    		}
-			pspdir[4+4*i+1] = (uint32_t)fd_stat.st_size;
+			pspdir[4+4*count+1] = (uint32_t)fd_stat.st_size;

-			pspdir[4+4*i+2] = pos + rom_base_address;
-			pspdir[4+4*i+3] = 0;
+			pspdir[4+4*count+2] = pos + rom_base_address;
+			pspdir[4+4*count+3] = 0;

if (pos + fd_stat.st_size > rom_size) {
    			printf("Error: Specified ROM size of %d"
@@ -404,11 +405,12 @@

close(fd);
    		pos = ALIGN(pos, 0x100U);
+			count++;
    	} else {
    		/* This APU doesn't have this firmware. */
    	}
    }
-	fill_psp_head(pspdir, i);
+	fill_psp_head(pspdir, count);
    return pos;
 }

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31726 )

Change subject: util/amdfwtool: Fix iteration of PSP firmwares
......................................................................

Patch Set 1: Code-Review+1

[Martin Roth (Code Review)]

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

Change subject: util/amdfwtool: Fix iteration of PSP firmwares
......................................................................

Patch Set 1: Code-Review+2

[Martin Roth (Code Review)]

Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31726 )

Change subject: util/amdfwtool: Fix iteration of PSP firmwares
......................................................................

util/amdfwtool: Fix iteration of PSP firmwares

Correct an oversight in the utility that attempts to match up eligible
PSP directory table entries with blob names passed on the command
line.  A 1:1 matchup of items shouldn't be assumed, so the i iterator
shouldn't be used to walk both lists.

This change has no effect on google/grunt (Family 15h Models 70h-7Fh),
but eliminates blank entries of all FF's on builds of amd/bettong
(F15h 60h-6Fh) and pcengines/apu2 (F16h 30h-3Fh).  Removal of entries
also affects the checksum accordingly.

TEST=Build before/after images for grunt, bettong, apu2, and diff
     hexdumps of the amdfw.rom files

Change-Id: I13e359d3cc6f5ce408bbf077feec3707ee2b3838
Signed-off-by: Marshall Dawson 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/31726
Reviewed-by: Paul Menzel 
Reviewed-by: Martin Roth 
Tested-by: build bot (Jenkins) 
---
M util/amdfwtool/amdfwtool.c
1 file changed, 13 insertions(+), 11 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Martin Roth: Looks good to me, approved
  Paul Menzel: Looks good to me, but someone else must approve

diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c
index 406ae05..234760d 100644
--- a/util/amdfwtool/amdfwtool.c
+++ b/util/amdfwtool/amdfwtool.c
@@ -355,17 +355,18 @@
    int fd;
    ssize_t bytes;
    struct stat fd_stat;
-	unsigned int i;
+	unsigned int i, count;
    uint32_t rom_base_address = 0xFFFFFFFF - rom_size + 1;

-	for (i = 0; fw_table[i].type != AMD_FW_INVALID; i++) {
+	for (i = 0, count = 0; fw_table[i].type != AMD_FW_INVALID; i++) {
    	if (fw_table[i].type == AMD_PSP_FUSE_CHAIN) {
-			pspdir[4+4*i+0] = fw_table[i].type;
-			pspdir[4+4*i+1] = 0xFFFFFFFF;
-			pspdir[4+4*i+2] = 1;
-			pspdir[4+4*i+3] = 0;
+			pspdir[4+4*count+0] = fw_table[i].type;
+			pspdir[4+4*count+1] = 0xFFFFFFFF;
+			pspdir[4+4*count+2] = 1;
+			pspdir[4+4*count+3] = 0;
+			count++;
    	} else if (fw_table[i].filename != NULL) {
-			pspdir[4+4*i+0] = fw_table[i].type;
+			pspdir[4+4*count+0] = fw_table[i].type;

fd = open(fw_table[i].filename, O_RDONLY);
    		if (fd < 0) {
@@ -378,10 +379,10 @@
    			free(base);
    			exit(1);
    		}
-			pspdir[4+4*i+1] = (uint32_t)fd_stat.st_size;
+			pspdir[4+4*count+1] = (uint32_t)fd_stat.st_size;

-			pspdir[4+4*i+2] = pos + rom_base_address;
-			pspdir[4+4*i+3] = 0;
+			pspdir[4+4*count+2] = pos + rom_base_address;
+			pspdir[4+4*count+3] = 0;

if (pos + fd_stat.st_size > rom_size) {
    			printf("Error: Specified ROM size of %d"
@@ -404,11 +405,12 @@

close(fd);
    		pos = ALIGN(pos, 0x100U);
+			count++;
    	} else {
    		/* This APU doesn't have this firmware. */
    	}
    }
-	fill_psp_head(pspdir, i);
+	fill_psp_head(pspdir, count);
    return pos;
 }