Hello Raul Rangel, Rob Barnes, Matt Papageorge,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/39861
to review the following change.
Change subject: amdfwtool: Allow for upto 16 APCB entries ......................................................................
amdfwtool: Allow for upto 16 APCB entries
Increase the number of allowed APCB entires in amdfwtool.
BUG=b:150455865 TEST=Boot Trembyle BRANCH=None
Signed-off-by: Rob Barnes robbarnes@google.com Change-Id: Ibdd2f2b9766735bc9aba98b5216e589b6cace238 Reviewed-on: https://chromium-review.googlesource.com/2084944 Reviewed-by: Matt Papageorge matt.papageorge@amd.corp-partner.google.com Reviewed-by: Raul E Rangel rrangel@chromium.org --- M util/amdfwtool/amdfwtool.c 1 file changed, 25 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/39861/1
diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index 4f1e8ba..9c50dbd 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -364,11 +364,33 @@ { .type = AMD_BIOS_APCB, .inst = 2, .level = BDT_BOTH }, { .type = AMD_BIOS_APCB, .inst = 3, .level = BDT_BOTH }, { .type = AMD_BIOS_APCB, .inst = 4, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 5, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 6, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 7, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 8, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 9, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 10, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 11, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 12, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 13, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 14, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 15, .level = BDT_BOTH }, { .type = AMD_BIOS_APCB_BK, .inst = 0, .level = BDT_BOTH }, { .type = AMD_BIOS_APCB_BK, .inst = 1, .level = BDT_BOTH }, { .type = AMD_BIOS_APCB_BK, .inst = 2, .level = BDT_BOTH }, { .type = AMD_BIOS_APCB_BK, .inst = 3, .level = BDT_BOTH }, { .type = AMD_BIOS_APCB_BK, .inst = 4, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 5, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 6, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 7, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 8, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 9, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 10, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 11, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 12, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 13, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 14, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 15, .level = BDT_BOTH }, { .type = AMD_BIOS_APOB, .level = BDT_BOTH }, { .type = AMD_BIOS_BIN, .reset = 1, .copy = 1, .zlib = 1, .level = BDT_BOTH }, @@ -470,7 +492,7 @@ bios_directory_entry entries[]; } bios_directory_table;
-#define MAX_BIOS_ENTRIES 0x1f +#define MAX_BIOS_ENTRIES 0x22
typedef struct _context { char *rom; /* target buffer, size of flash device */ @@ -1004,7 +1026,8 @@ }
if (count > MAX_BIOS_ENTRIES) { - printf("Error: BIOS entries exceeds max allowed items\n"); + printf("Error: BIOS entries (%d) exceeds max allowed items " + "(%d)\n", count, MAX_BIOS_ENTRIES); free(ctx->rom); exit(1); }
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39861 )
Change subject: amdfwtool: Allow for upto 16 APCB entries ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39861/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39861/1//COMMIT_MSG@9 PS1, Line 9: entires entries
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39861 )
Change subject: amdfwtool: Allow for upto 16 APCB entries ......................................................................
Patch Set 1: Code-Review+2
Hello build bot (Jenkins), Raul Rangel, Rob Barnes, Matt Papageorge,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39861
to look at the new patch set (#2).
Change subject: amdfwtool: Allow for upto 16 APCB entries ......................................................................
amdfwtool: Allow for upto 16 APCB entries
Increase the number of allowed APCB entries in amdfwtool.
BUG=b:150455865 TEST=Boot Trembyle BRANCH=None
Signed-off-by: Rob Barnes robbarnes@google.com Change-Id: Ibdd2f2b9766735bc9aba98b5216e589b6cace238 Reviewed-on: https://chromium-review.googlesource.com/2084944 Reviewed-by: Matt Papageorge matt.papageorge@amd.corp-partner.google.com Reviewed-by: Raul E Rangel rrangel@chromium.org --- M util/amdfwtool/amdfwtool.c 1 file changed, 25 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/39861/2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39861 )
Change subject: amdfwtool: Allow for upto 16 APCB entries ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39861/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39861/1//COMMIT_MSG@9 PS1, Line 9: entires
entries
Done
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39861 )
Change subject: amdfwtool: Allow for upto 16 APCB entries ......................................................................
Patch Set 2: Code-Review+1
Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39861 )
Change subject: amdfwtool: Allow for upto 16 APCB entries ......................................................................
Patch Set 2: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39861 )
Change subject: amdfwtool: Allow for upto 16 APCB entries ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/39861/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39861/2//COMMIT_MSG@7 PS2, Line 7: upto nit: up to
Hello build bot (Jenkins), Raul Rangel, Angel Pons, Rob Barnes, Matt Papageorge,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39861
to look at the new patch set (#3).
Change subject: amdfwtool: Allow for up to 16 APCB entries ......................................................................
amdfwtool: Allow for up to 16 APCB entries
Increase the number of allowed APCB entries in amdfwtool.
BUG=b:150455865 TEST=Boot Trembyle BRANCH=None
Signed-off-by: Rob Barnes robbarnes@google.com Change-Id: Ibdd2f2b9766735bc9aba98b5216e589b6cace238 Reviewed-on: https://chromium-review.googlesource.com/2084944 Reviewed-by: Matt Papageorge matt.papageorge@amd.corp-partner.google.com Reviewed-by: Raul E Rangel rrangel@chromium.org --- M util/amdfwtool/amdfwtool.c 1 file changed, 25 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/39861/3
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39861 )
Change subject: amdfwtool: Allow for up to 16 APCB entries ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39861/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39861/2//COMMIT_MSG@7 PS2, Line 7: upto
nit: up to
Done
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39861 )
Change subject: amdfwtool: Allow for up to 16 APCB entries ......................................................................
amdfwtool: Allow for up to 16 APCB entries
Increase the number of allowed APCB entries in amdfwtool.
BUG=b:150455865 TEST=Boot Trembyle BRANCH=None
Signed-off-by: Rob Barnes robbarnes@google.com Change-Id: Ibdd2f2b9766735bc9aba98b5216e589b6cace238 Reviewed-on: https://chromium-review.googlesource.com/2084944 Reviewed-by: Matt Papageorge matt.papageorge@amd.corp-partner.google.com Reviewed-by: Raul E Rangel rrangel@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/39861 Reviewed-by: Matt Papageorge matthewpapa07@gmail.com Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Raul Rangel rrangel@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M util/amdfwtool/amdfwtool.c 1 file changed, 25 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Raul Rangel: Looks good to me, approved Angel Pons: Looks good to me, approved Matt Papageorge: Looks good to me, but someone else must approve Rob Barnes: Looks good to me, but someone else must approve
diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index 4f1e8ba..9c50dbd 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -364,11 +364,33 @@ { .type = AMD_BIOS_APCB, .inst = 2, .level = BDT_BOTH }, { .type = AMD_BIOS_APCB, .inst = 3, .level = BDT_BOTH }, { .type = AMD_BIOS_APCB, .inst = 4, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 5, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 6, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 7, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 8, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 9, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 10, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 11, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 12, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 13, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 14, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB, .inst = 15, .level = BDT_BOTH }, { .type = AMD_BIOS_APCB_BK, .inst = 0, .level = BDT_BOTH }, { .type = AMD_BIOS_APCB_BK, .inst = 1, .level = BDT_BOTH }, { .type = AMD_BIOS_APCB_BK, .inst = 2, .level = BDT_BOTH }, { .type = AMD_BIOS_APCB_BK, .inst = 3, .level = BDT_BOTH }, { .type = AMD_BIOS_APCB_BK, .inst = 4, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 5, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 6, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 7, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 8, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 9, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 10, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 11, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 12, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 13, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 14, .level = BDT_BOTH }, + { .type = AMD_BIOS_APCB_BK, .inst = 15, .level = BDT_BOTH }, { .type = AMD_BIOS_APOB, .level = BDT_BOTH }, { .type = AMD_BIOS_BIN, .reset = 1, .copy = 1, .zlib = 1, .level = BDT_BOTH }, @@ -470,7 +492,7 @@ bios_directory_entry entries[]; } bios_directory_table;
-#define MAX_BIOS_ENTRIES 0x1f +#define MAX_BIOS_ENTRIES 0x22
typedef struct _context { char *rom; /* target buffer, size of flash device */ @@ -1004,7 +1026,8 @@ }
if (count > MAX_BIOS_ENTRIES) { - printf("Error: BIOS entries exceeds max allowed items\n"); + printf("Error: BIOS entries (%d) exceeds max allowed items " + "(%d)\n", count, MAX_BIOS_ENTRIES); free(ctx->rom); exit(1); }
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39861 )
Change subject: amdfwtool: Allow for up to 16 APCB entries ......................................................................
Patch Set 4:
Automatic boot test returned (PASS/FAIL/TOTAL): 7/0/7 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1855 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1854 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1853 Non-emulation targets: HP_Z220_SFF_WORKSTATION using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1859 HP_Z220_SFF_WORKSTATION using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1858 HP_COMPAQ_8200_ELITE_SFF_PC using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1857 HP_COMPAQ_8200_ELITE_SFF_PC using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1856
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Felix Held (Code Review)
-
Matt Papageorge (Code Review)
-
Paul Menzel (Code Review)
-
Raul Rangel (Code Review)
-
Rob Barnes (Code Review)