Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33396
Change subject: util/amdfwtool: Add argument for soft fuse override ......................................................................
util/amdfwtool: Add argument for soft fuse override
Allow the soc build to pass a soft fuse value to the utility. This helps maintain compatibility across PSP generations.
Add a generic 'other' item to the amd_fw_entry structure that may be used by non-fuse entries in the future.
TEST=Verify google/grunt amdfw.rom unchanged before and after. Compare internal board using override before and after.
Change-Id: I26223f0b42ad28c43d9bd87419a2a8f719ee91cb Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 26 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/33396/1
diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index 4a13b29..51e913c 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -184,6 +184,7 @@ printf("-u | --trustletkey <FILE> Add trustletkey\n"); printf("-w | --smufirmware2 <FILE> Add smufirmware2\n"); printf("-m | --smuscs <FILE> Add smuscs\n"); + printf("-T | --soft-fuse <HEX_VAL> Override default soft fuse values\n"); printf("\n-o | --output <filename> output filename\n"); printf("-f | --flashsize <HEX_VAL> ROM size in bytes\n"); printf(" size must be larger than %dKB\n", @@ -218,6 +219,7 @@ amd_fw_type type; uint8_t subprog; char *filename; + uint64_t other; } amd_fw_entry;
static amd_fw_entry amd_psp_fw_table[] = { @@ -450,7 +452,10 @@ pspdir->entries[count].subprog = fw_table[i].subprog; pspdir->entries[count].rsvd = 0; pspdir->entries[count].size = 0xFFFFFFFF; - pspdir->entries[count].addr = 1; + if (fw_table[i].other) + pspdir->entries[count].addr = fw_table[i].other; + else + pspdir->entries[count].addr = 1; count++; } else if (fw_table[i].filename != NULL) { bytes = copy_blob(BUFF_CURRENT(*ctx), @@ -483,7 +488,7 @@ fill_dir_header(pspdir, count, PSP_COOKIE); }
-static const char *optstring = "x:i:g:AS:p:b:s:r:k:c:n:d:t:u:w:m:o:f:l:h"; +static const char *optstring = "x:i:g:AS:p:b:s:r:k:c:n:d:t:u:w:m:T:o:f:l:h";
static struct option long_options[] = { {"xhci", required_argument, 0, 'x' }, @@ -504,6 +509,7 @@ {"trustletkey", required_argument, 0, 'u' }, {"smufirmware2", required_argument, 0, 'w' }, {"smuscs", required_argument, 0, 'm' }, + {"soft-fuse", required_argument, 0, 'T' }, {"output", required_argument, 0, 'o' }, {"flashsize", required_argument, 0, 'f' }, {"location", required_argument, 0, 'l' }, @@ -512,6 +518,20 @@ {NULL, 0, 0, 0 } };
+static void register_fw_fuse(char *str) +{ + int i; + char *tmp; + + for (i = 0; i < sizeof(amd_psp_fw_table) / sizeof(amd_fw_entry); i++) { + if (amd_psp_fw_table[i].type != AMD_PSP_FUSE_CHAIN) + continue; + + amd_psp_fw_table[i].other = strtoull(str, &tmp, 16); + return; + } +} + static void register_fw_filename(amd_fw_type type, uint8_t sub, char filename[]) { unsigned int i; @@ -635,6 +655,10 @@ register_fw_filename(AMD_FW_PSP_SMUSCS, sub, optarg); sub = 0; break; + case 'T': + register_fw_fuse(optarg); + sub = 0; + break; case 'o': output = optarg; break;
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33396 )
Change subject: util/amdfwtool: Add argument for soft fuse override ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33396/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/#/c/33396/1/util/amdfwtool/amdfwtool.c@187 PS1, Line 187: printf("-T | --soft-fuse <HEX_VAL> Override default soft fuse values\n"); line over 80 characters
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33396 )
Change subject: util/amdfwtool: Add argument for soft fuse override ......................................................................
Patch Set 1: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33396 )
Change subject: util/amdfwtool: Add argument for soft fuse override ......................................................................
Patch Set 1: Code-Review-1
(2 comments)
https://review.coreboot.org/#/c/33396/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/#/c/33396/1/util/amdfwtool/amdfwtool.c@458 PS1, Line 458: pspdir->entries[count].addr = 1; Ths makes '-T 0' silently act like '-T 1'. Also I wonder if the default value is really compatible across generations, so maybe this entry should only be injected in the directory when the commandline option is present.
https://review.coreboot.org/#/c/33396/1/util/amdfwtool/amdfwtool.c@530 PS1, Line 530: amd_psp_fw_table[i].other = strtoull(str, &tmp, 16); Assigned but unused tmp, should work with NULL as 2nd parameter.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33396 )
Change subject: util/amdfwtool: Add argument for soft fuse override ......................................................................
Patch Set 1: -Code-Review
Remove my +2 to get Kyösti's comment addressed.
Hello Kyösti Mälkki, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33396
to look at the new patch set (#2).
Change subject: util/amdfwtool: Add argument for soft fuse override ......................................................................
util/amdfwtool: Add argument for soft fuse override
Allow the soc build to pass a soft fuse value to the utility. This helps maintain compatibility across PSP generations.
Add a generic 'other' item to the amd_fw_entry structure that may be used by non-fuse entries in the future.
TEST=Verify google/grunt amdfw.rom unchanged before and after. Compare internal board using override before and after.
Change-Id: I26223f0b42ad28c43d9bd87419a2a8f719ee91cb Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 29 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/33396/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33396 )
Change subject: util/amdfwtool: Add argument for soft fuse override ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33396/2/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/#/c/33396/2/util/amdfwtool/amdfwtool.c@189 PS2, Line 189: printf("-T | --soft-fuse <HEX_VAL> Override default soft fuse values\n"); line over 80 characters
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33396 )
Change subject: util/amdfwtool: Add argument for soft fuse override ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/33396/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/#/c/33396/1/util/amdfwtool/amdfwtool.c@458 PS1, Line 458: pspdir->entries[count].addr = 1;
Ths makes '-T 0' silently act like '-T 1'. […]
Done
https://review.coreboot.org/#/c/33396/1/util/amdfwtool/amdfwtool.c@530 PS1, Line 530: amd_psp_fw_table[i].other = strtoull(str, &tmp, 16);
Assigned but unused tmp, should work with NULL as 2nd parameter.
Done
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33396 )
Change subject: util/amdfwtool: Add argument for soft fuse override ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/33396/2/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/#/c/33396/2/util/amdfwtool/amdfwtool.c@83 PS2, Line 83: #define DEFAULT_SOFT_FUSE_CHAIN "0x1" I have 54267 1.05 document under NDA. Can we discuss if the default value of setting bit 0 is something you want to ship with?
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33396 )
Change subject: util/amdfwtool: Add argument for soft fuse override ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/33396/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/#/c/33396/1/util/amdfwtool/amdfwtool.c@458 PS1, Line 458: pspdir->entries[count].addr = 1;
Done
Thanks for catching, BTW.
https://review.coreboot.org/#/c/33396/2/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/#/c/33396/2/util/amdfwtool/amdfwtool.c@83 PS2, Line 83: #define DEFAULT_SOFT_FUSE_CHAIN "0x1"
I have 54267 1.05 document under NDA. […]
I assume you recognized my plan was to leave all platforms' current settings alone but I'm going to be overriding it for Picasso.
I've thought b0=0 would be an improvement and surely some customers may prefer it. However, I'm not sure about its practical benefits. You know what the bit does, but if an attacker has physical access and the ability to rewrite flash, then they just change the bit and update the checksum. BTW, I'm implying what the requests for security features certain customers used to request while I was at AMD, and this wouldn't be sufficient.
I've also noted that AMD ships CRB images with b0=1 (there's probably nothing wrong with that, especially for what CRBs are intended for). But also, I just checked the BIOS on my main system and b0=1 there also. So, I just haven't worried about it.
By the way, I'm OK making the default 0 after a quick smoke test on stoney (and I'll infer CZ will behave the same). Have you run that way? I'm not sure how Mullins would behave with the minimal PSP implementation.
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33396 )
Change subject: util/amdfwtool: Add argument for soft fuse override ......................................................................
util/amdfwtool: Add argument for soft fuse override
Allow the soc build to pass a soft fuse value to the utility. This helps maintain compatibility across PSP generations.
Add a generic 'other' item to the amd_fw_entry structure that may be used by non-fuse entries in the future.
TEST=Verify google/grunt amdfw.rom unchanged before and after. Compare internal board using override before and after.
Change-Id: I26223f0b42ad28c43d9bd87419a2a8f719ee91cb Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33396 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 29 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, approved
diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index 4a13b29..a3a692d 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -80,6 +80,8 @@ #define TABLE_ALIGNMENT 0x1000U #define BLOB_ALIGNMENT 0x100U
+#define DEFAULT_SOFT_FUSE_CHAIN "0x1" + #define EMBEDDED_FW_SIGNATURE 0x55aa55aa #define PSP_COOKIE 0x50535024 /* 'PSP$' */ #define PSP2_COOKIE 0x50535032 /* 'PSP2' */ @@ -184,6 +186,7 @@ printf("-u | --trustletkey <FILE> Add trustletkey\n"); printf("-w | --smufirmware2 <FILE> Add smufirmware2\n"); printf("-m | --smuscs <FILE> Add smuscs\n"); + printf("-T | --soft-fuse <HEX_VAL> Override default soft fuse values\n"); printf("\n-o | --output <filename> output filename\n"); printf("-f | --flashsize <HEX_VAL> ROM size in bytes\n"); printf(" size must be larger than %dKB\n", @@ -218,6 +221,7 @@ amd_fw_type type; uint8_t subprog; char *filename; + uint64_t other; } amd_fw_entry;
static amd_fw_entry amd_psp_fw_table[] = { @@ -450,7 +454,7 @@ pspdir->entries[count].subprog = fw_table[i].subprog; pspdir->entries[count].rsvd = 0; pspdir->entries[count].size = 0xFFFFFFFF; - pspdir->entries[count].addr = 1; + pspdir->entries[count].addr = fw_table[i].other; count++; } else if (fw_table[i].filename != NULL) { bytes = copy_blob(BUFF_CURRENT(*ctx), @@ -483,7 +487,7 @@ fill_dir_header(pspdir, count, PSP_COOKIE); }
-static const char *optstring = "x:i:g:AS:p:b:s:r:k:c:n:d:t:u:w:m:o:f:l:h"; +static const char *optstring = "x:i:g:AS:p:b:s:r:k:c:n:d:t:u:w:m:T:o:f:l:h";
static struct option long_options[] = { {"xhci", required_argument, 0, 'x' }, @@ -504,6 +508,7 @@ {"trustletkey", required_argument, 0, 'u' }, {"smufirmware2", required_argument, 0, 'w' }, {"smuscs", required_argument, 0, 'm' }, + {"soft-fuse", required_argument, 0, 'T' }, {"output", required_argument, 0, 'o' }, {"flashsize", required_argument, 0, 'f' }, {"location", required_argument, 0, 'l' }, @@ -512,6 +517,19 @@ {NULL, 0, 0, 0 } };
+static void register_fw_fuse(char *str) +{ + int i; + + for (i = 0; i < sizeof(amd_psp_fw_table) / sizeof(amd_fw_entry); i++) { + if (amd_psp_fw_table[i].type != AMD_PSP_FUSE_CHAIN) + continue; + + amd_psp_fw_table[i].other = strtoull(str, NULL, 16); + return; + } +} + static void register_fw_filename(amd_fw_type type, uint8_t sub, char filename[]) { unsigned int i; @@ -543,6 +561,7 @@ embedded_firmware *amd_romsig; psp_directory_table *pspdir; int comboable = 0; + int fuse_defined = 0; int targetfd; char *output = NULL; context ctx = { @@ -635,6 +654,11 @@ register_fw_filename(AMD_FW_PSP_SMUSCS, sub, optarg); sub = 0; break; + case 'T': + register_fw_fuse(optarg); + fuse_defined = 1; + sub = 0; + break; case 'o': output = optarg; break; @@ -663,6 +687,9 @@ } }
+ if (!fuse_defined) + register_fw_fuse(DEFAULT_SOFT_FUSE_CHAIN); + if (!output) { printf("Error: Output value is not specified.\n\n"); retval = 1;
-
build bot (Jenkins) (Code Review) -
Kyösti Mälkki (Code Review)
-
Marshall Dawson (Code Review)
-
Martin Roth (Code Review)