Martin Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42043 )
Change subject: util/amdfwtool: update to allow building in any location ......................................................................
util/amdfwtool: update to allow building in any location
For the verstage-on-PSP implementation, we need 2 additional copies of the AMD firmware tables at non-standard locations. These are for RW-A & RW-B fmap regions. This change allows us to build the AMD firmware tables into those regions.
BUG=b:148767300 TEST=boot with psp_verstage, verify boot location
Signed-off-by: Martin Roth martin@coreboot.org Original-Signed-off-by: Martin Roth martinroth@chromium.org Original-Change-Id: I2b591b50e9b179fdfaead46ff93722fa2a155e9c Original-Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+... Original-Reviewed-by: Simon Glass sjg@chromium.org Change-Id: I7f841db8617b953dc671a9c12576145f85263581 --- M util/amdfwtool/amdfwtool.c 1 file changed, 30 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/42043/1
diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index 71da6fb..e6fa220 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -211,6 +211,7 @@ MIN_ROM_KB); printf(" and must a multiple of 1024\n"); printf("-l | --location Location of Directory\n"); + printf("-q | --anywhere Use any 64-byte aligned addr for Directory\n"); printf("-h | --help show this help\n"); }
@@ -1021,8 +1022,8 @@
fill_dir_header(biosdir, count, cookie); } -// Unused values: CDEPqR -static const char *optstring = "x:i:g:AMS:p:b:s:r:k:c:n:d:t:u:w:m:T:z:J:B:K:L:Y:N:UW:I:a:Q:V:e:v:j:y:G:O:X:F:H:o:f:l:hZ:"; +// Unused values: CDEPR +static const char *optstring = "x:i:g:AMS:p:b:s:r:k:c:n:d:t:u:w:m:T:z:J:B:K:L:Y:N:UW:I:a:Q:V:e:v:j:y:G:O:X:F:H:o:f:l:hZ:q";
static struct option long_options[] = { {"xhci", required_argument, 0, 'x' }, @@ -1073,6 +1074,7 @@ {"output", required_argument, 0, 'o' }, {"flashsize", required_argument, 0, 'f' }, {"location", required_argument, 0, 'l' }, + {"anywhere", no_argument, 0, 'q' }, {"help", no_argument, 0, 'h' }, {NULL, 0, 0, 0 } }; @@ -1177,6 +1179,7 @@ uint8_t sub = 0, instance = 0; int abl_image = 0; uint32_t dir_location = 0; + uint8_t any_location = 0; uint32_t romsig_offset; uint32_t rom_base_address; int multi = 0; @@ -1392,6 +1395,9 @@ retval = 1; } break; + case 'q': + any_location = 1; + break;
case 'h': usage(); @@ -1434,20 +1440,28 @@ return 1; }
- switch (dir_location) { - case 0: /* Fall through */ - case 0xFFFA0000: /* Fall through */ - case 0xFFF20000: /* Fall through */ - case 0xFFE20000: /* Fall through */ - case 0xFFC20000: /* Fall through */ - case 0xFF820000: /* Fall through */ - case 0xFF020000: /* Fall through */ - break; - default: - printf("Error: Invalid Directory location.\n"); - printf(" Valid locations are 0xFFFA0000, 0xFFF20000,\n"); - printf(" 0xFFE20000, 0xFFC20000, 0xFF820000, 0xFF020000\n"); - return 1; + if (any_location) { + if (dir_location & 0x3f) { + printf("Error: Invalid Directory location.\n"); + printf(" Valid locations are 64-byte aligned\n"); + return 1; + } + } else { + switch (dir_location) { + case 0: /* Fall through */ + case 0xFFFA0000: /* Fall through */ + case 0xFFF20000: /* Fall through */ + case 0xFFE20000: /* Fall through */ + case 0xFFC20000: /* Fall through */ + case 0xFF820000: /* Fall through */ + case 0xFF020000: /* Fall through */ + break; + default: + printf("Error: Invalid Directory location.\n"); + printf(" Valid locations are 0xFFFA0000, 0xFFF20000,\n"); + printf(" 0xFFE20000, 0xFFC20000, 0xFF820000, 0xFF020000\n"); + return 1; + } }
ctx.rom = malloc(ctx.rom_size);
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42043 )
Change subject: util/amdfwtool: update to allow building in any location ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42043/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42043/1/util/amdfwtool/amdfwtool.c@... PS1, Line 1026: static const char *optstring = "x:i:g:AMS:p:b:s:r:k:c:n:d:t:u:w:m:T:z:J:B:K:L:Y:N:UW:I:a:Q:V:e:v:j:y:G:O:X:F:H:o:f:l:hZ:q"; line over 96 characters
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42043 )
Change subject: util/amdfwtool: update to allow building in any location ......................................................................
Patch Set 1: Code-Review+2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42043 )
Change subject: util/amdfwtool: update to allow building in any location ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42043/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42043/1/util/amdfwtool/amdfwtool.c@... PS1, Line 1182: uint8_t bool?
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42043 )
Change subject: util/amdfwtool: update to allow building in any location ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42043/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42043/1/util/amdfwtool/amdfwtool.c@... PS1, Line 1182: uint8_t
bool?
Meh, Call me old, but I don't care to use bool. I really don't see the point. It's not like it's going to save memory or code soace. The smallest chunk of memory that can be addressed in a register is 8 bits. And it's either 0 or not zero.
Really, I think it should just be an int if anything, and let the compiler decide what to use.
Can you tell me what the advantage is to using bool (other than making it absolutely certain that it's either a 0 or 1 - and I don't really care here).
I mean, I'll change it if that's the new coreboot standard, but I think there are LOTS of places throughout the code that COULD use bool that don't.
Hello Jason Glenesk, build bot (Jenkins), Raul Rangel, Marshall Dawson, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42043
to look at the new patch set (#2).
Change subject: util/amdfwtool: update to allow building in any location ......................................................................
util/amdfwtool: update to allow building in any location
For the verstage-on-PSP implementation, we need 2 additional copies of the AMD firmware tables at non-standard locations. These are for RW-A & RW-B fmap regions. This change allows us to build the AMD firmware tables into those regions.
BUG=b:148767300 TEST=boot with psp_verstage, verify boot location
Signed-off-by: Martin Roth martin@coreboot.org Original-Signed-off-by: Martin Roth martinroth@chromium.org Original-Change-Id: I2b591b50e9b179fdfaead46ff93722fa2a155e9c Original-Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+... Original-Reviewed-by: Simon Glass sjg@chromium.org Change-Id: I7f841db8617b953dc671a9c12576145f85263581 --- M util/amdfwtool/amdfwtool.c 1 file changed, 31 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/42043/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42043 )
Change subject: util/amdfwtool: update to allow building in any location ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42043/2/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42043/2/util/amdfwtool/amdfwtool.c@... PS2, Line 1027: static const char *optstring = "x:i:g:AMS:p:b:s:r:k:c:n:d:t:u:w:m:T:z:J:B:K:L:Y:N:UW:I:a:Q:V:e:v:j:y:G:O:X:F:H:o:f:l:hZ:q"; line over 96 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42043 )
Change subject: util/amdfwtool: update to allow building in any location ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42043/3/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42043/3/util/amdfwtool/amdfwtool.c@... PS3, Line 1027: static const char *optstring = "x:i:g:AMS:p:b:s:r:k:c:n:d:t:u:w:m:T:z:J:B:K:L:Y:N:UW:I:a:Q:V:e:v:j:y:G:O:X:F:H:o:f:l:hZ:q"; line over 96 characters
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42043 )
Change subject: util/amdfwtool: update to allow building in any location ......................................................................
Patch Set 3: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42043 )
Change subject: util/amdfwtool: update to allow building in any location ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42043/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42043/1/util/amdfwtool/amdfwtool.c@... PS1, Line 1182: uint8_t
Meh, Call me old, but I don't care to use bool. I really don't see the point. […]
Just recently learned about resentments of C people of using `bool`. I’d use `int` then.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42043 )
Change subject: util/amdfwtool: update to allow building in any location ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42043/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42043/1/util/amdfwtool/amdfwtool.c@... PS1, Line 1182: uint8_t
Just recently learned about resentments of C people of using `bool`. I’d use `int` then.
for me it's about semantics here. when a variable has bool type, it's clear that it's either zero or non-zero and that's all the code cares about. if it's an int, it's not clear if the actual values does matter in some place or if it only matters if it's zero or non-zero. so for me it makes it much easiert to see what's going on in the code. C doesn't have much of a proper type system, but i like to at least use what's there. I don't insist on this change, but better readability also means better maintainability and less chances of breaking things in the future for me.
and using int instead of an unsigned type is a bad idea. i think i've seen some bitfield structs where the field type was signed and the width 1 bit, which is even more undefined behaviour than bitfield structs alone
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42043 )
Change subject: util/amdfwtool: update to allow building in any location ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42043/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42043/1/util/amdfwtool/amdfwtool.c@... PS1, Line 1182: uint8_t
Meh, Call me old, but I don't care to use bool. I really don't see the point. […]
I still don't see the need in this case, but I guess it helps the compiler know what's expected, and in some situations that could help find bugs.
Martin Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42043 )
Change subject: util/amdfwtool: update to allow building in any location ......................................................................
util/amdfwtool: update to allow building in any location
For the verstage-on-PSP implementation, we need 2 additional copies of the AMD firmware tables at non-standard locations. These are for RW-A & RW-B fmap regions. This change allows us to build the AMD firmware tables into those regions.
BUG=b:148767300 TEST=boot with psp_verstage, verify boot location
Signed-off-by: Martin Roth martin@coreboot.org Original-Signed-off-by: Martin Roth martinroth@chromium.org Original-Change-Id: I2b591b50e9b179fdfaead46ff93722fa2a155e9c Original-Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+... Original-Reviewed-by: Simon Glass sjg@chromium.org Change-Id: I7f841db8617b953dc671a9c12576145f85263581 Reviewed-on: https://review.coreboot.org/c/coreboot/+/42043 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Raul Rangel rrangel@chromium.org --- M util/amdfwtool/amdfwtool.c 1 file changed, 31 insertions(+), 16 deletions(-)
Approvals: build bot (Jenkins): Verified Raul Rangel: Looks good to me, approved
diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index 71da6fb..a522295 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -48,6 +48,7 @@
#include <fcntl.h> #include <errno.h> +#include <stdbool.h> #include <stdio.h> #include <sys/stat.h> #include <sys/types.h> @@ -211,6 +212,7 @@ MIN_ROM_KB); printf(" and must a multiple of 1024\n"); printf("-l | --location Location of Directory\n"); + printf("-q | --anywhere Use any 64-byte aligned addr for Directory\n"); printf("-h | --help show this help\n"); }
@@ -1021,8 +1023,8 @@
fill_dir_header(biosdir, count, cookie); } -// Unused values: CDEPqR -static const char *optstring = "x:i:g:AMS:p:b:s:r:k:c:n:d:t:u:w:m:T:z:J:B:K:L:Y:N:UW:I:a:Q:V:e:v:j:y:G:O:X:F:H:o:f:l:hZ:"; +// Unused values: CDEPR +static const char *optstring = "x:i:g:AMS:p:b:s:r:k:c:n:d:t:u:w:m:T:z:J:B:K:L:Y:N:UW:I:a:Q:V:e:v:j:y:G:O:X:F:H:o:f:l:hZ:q";
static struct option long_options[] = { {"xhci", required_argument, 0, 'x' }, @@ -1073,6 +1075,7 @@ {"output", required_argument, 0, 'o' }, {"flashsize", required_argument, 0, 'f' }, {"location", required_argument, 0, 'l' }, + {"anywhere", no_argument, 0, 'q' }, {"help", no_argument, 0, 'h' }, {NULL, 0, 0, 0 } }; @@ -1177,6 +1180,7 @@ uint8_t sub = 0, instance = 0; int abl_image = 0; uint32_t dir_location = 0; + bool any_location = 0; uint32_t romsig_offset; uint32_t rom_base_address; int multi = 0; @@ -1392,6 +1396,9 @@ retval = 1; } break; + case 'q': + any_location = 1; + break;
case 'h': usage(); @@ -1434,20 +1441,28 @@ return 1; }
- switch (dir_location) { - case 0: /* Fall through */ - case 0xFFFA0000: /* Fall through */ - case 0xFFF20000: /* Fall through */ - case 0xFFE20000: /* Fall through */ - case 0xFFC20000: /* Fall through */ - case 0xFF820000: /* Fall through */ - case 0xFF020000: /* Fall through */ - break; - default: - printf("Error: Invalid Directory location.\n"); - printf(" Valid locations are 0xFFFA0000, 0xFFF20000,\n"); - printf(" 0xFFE20000, 0xFFC20000, 0xFF820000, 0xFF020000\n"); - return 1; + if (any_location) { + if (dir_location & 0x3f) { + printf("Error: Invalid Directory location.\n"); + printf(" Valid locations are 64-byte aligned\n"); + return 1; + } + } else { + switch (dir_location) { + case 0: /* Fall through */ + case 0xFFFA0000: /* Fall through */ + case 0xFFF20000: /* Fall through */ + case 0xFFE20000: /* Fall through */ + case 0xFFC20000: /* Fall through */ + case 0xFF820000: /* Fall through */ + case 0xFF020000: /* Fall through */ + break; + default: + printf("Error: Invalid Directory location.\n"); + printf(" Valid locations are 0xFFFA0000, 0xFFF20000,\n"); + printf(" 0xFFE20000, 0xFFC20000, 0xFF820000, 0xFF020000\n"); + return 1; + } }
ctx.rom = malloc(ctx.rom_size);
-
build bot (Jenkins) (Code Review) -
Felix Held (Code Review)
-
Martin Roth (Code Review)
-
Paul Menzel (Code Review)
-
Raul Rangel (Code Review)