Change in coreboot[master]: util/cbfstool: Treat region offsets differently than absolute addresses
Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47829 )
Change subject: util/cbfstool: Treat region offsets differently than absolute addresses ......................................................................
util/cbfstool: Treat region offsets differently than absolute addresses
cbfstool overloads baseaddress to represent multiple things: 1. Address in SPI flash space 2. Address in host space (for x86 platforms) 3. Offset from end of region (accepted as negative number)
This was done so that the different functions that use these addresses/offsets don't need to be aware of what the value represents and can use the helper functions convert_to_from* to get the required values.
Thus, even if the user provides a negative value to represent offset from end of region, it was stored as an unsigned integer. There are special checks in convert_to_from_top_aligned which guesses if the value provided is really an offset from the end of region and converts it to an offset from start of region.
This has worked okay until now for x86 platforms because there is a single fixed decode window mapping the SPI flash to host address space. However, going forward new platforms might need to support more decode windows that are not contiguous in the host space. Thus, it is important to distinguish between offsets from end of region and addresses in host/SPI flash space and treat them separately.
As a first step towards supporting this requirement for multiple decode windows on new platforms, this change handles the negative offset provided as input in dispatch_command before the requested cbfs operation is performed.
This change adds baseaddress_input, headeroffset_input and cbfsoffset_input to struct param and converts them to offsets from start of region before storing into baseaddress, headeroffset and cbfsoffset if the inputs are negative.
In follow up changes, cbfstool will be extended to add support for multiple decode windows.
TEST=Verified using abuild with timeless option for all coreboot boards that there is no change in the resultant coreboot.rom file.
Change-Id: Ib74a7e6ed9e88fbc5489640d73bedac14872953f Signed-off-by: Furquan Shaikh furquan@google.com --- M util/cbfstool/cbfs_image.c M util/cbfstool/cbfstool.c 2 files changed, 54 insertions(+), 41 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/47829/1
diff --git a/util/cbfstool/cbfs_image.c b/util/cbfstool/cbfs_image.c index a042f0d..3f1bf43 100644 --- a/util/cbfstool/cbfs_image.c +++ b/util/cbfstool/cbfs_image.c @@ -277,14 +277,6 @@ bootblock_offset, bootblock->size, header_offset, sizeof(image->header), entries_offset);
- // Adjust legacy top-aligned address to ROM offset. - if (IS_TOP_ALIGNED_ADDRESS(entries_offset)) - entries_offset = size + (int32_t)entries_offset; - if (IS_TOP_ALIGNED_ADDRESS(bootblock_offset)) - bootblock_offset = size + (int32_t)bootblock_offset; - if (IS_TOP_ALIGNED_ADDRESS(header_offset)) - header_offset = size + (int32_t)header_offset; - DEBUG("cbfs_create_image: (real offset) bootblock=0x%x, " "header=0x%x, entries_offset=0x%x\n", bootblock_offset, header_offset, entries_offset); diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index d2df1cc..c719b8f 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -47,15 +47,22 @@ uint64_t u64val; uint32_t type; uint32_t baseaddress; + /* + * Input can be negative. It will be transformed to corresponding region offset and + * stored in baseaddress. + */ + long long int baseaddress_input; uint32_t baseaddress_assigned; uint32_t loadaddress; uint32_t headeroffset; + long long int headeroffset_input; uint32_t headeroffset_assigned; uint32_t entrypoint; uint32_t size; uint32_t alignment; uint32_t pagesize; uint32_t cbfsoffset; + long long int cbfsoffset_input; uint32_t cbfsoffset_assigned; uint32_t arch; uint32_t padding; @@ -102,8 +109,7 @@
/* * Converts between offsets from the start of the specified image region and - * "top-aligned" offsets from the top of the entire boot media. See comment - * below for convert_to_from_top_aligned() about forming addresses. + * "top-aligned" offsets from the top of the entire boot media. */ static unsigned convert_to_from_absolute_top_aligned( const struct buffer *region, unsigned offset) @@ -116,25 +122,26 @@ }
/* - * Converts between offsets from the start of the specified image region and - * "top-aligned" offsets from the top of the image region. Works in either - * direction: pass in one type of offset and receive the other type. - * N.B. A top-aligned offset is always a positive number, and should not be - * confused with a top-aligned *address*, which is its arithmetic inverse. */ -static unsigned convert_to_from_top_aligned(const struct buffer *region, - unsigned offset) + * This function takes offset value which represents the offset from one end of the region and + * returns the offset from the other end of the region. offset is expected to be positive. + */ +static unsigned convert_region_offset(unsigned offset) { - assert(region); + size_t size;
- /* Cover the situation where a negative base address is given by the - * user. Callers of this function negate it, so it'll be a positive - * number smaller than the region. - */ - if ((offset > 0) && (offset < region->size)) { - return region->size - offset; + if (param.size) { + size = param.size; + } else { + assert(param.image_region); + size = param.image_region->size; }
- return convert_to_from_absolute_top_aligned(region, offset); + if (size < offset) { + ERROR("Cannot convert region offset (size=0x%zx, offset=0x%x\n", size, offset); + return 0; + } + + return size - offset; }
static int do_cbfs_locate(int32_t *cbfs_addr, size_t metadata_size, @@ -242,10 +249,6 @@ goto done; }
- if (IS_TOP_ALIGNED_ADDRESS(offset)) - offset = convert_to_from_top_aligned(param.image_region, - -offset); - header = cbfs_create_file_header(CBFS_COMPONENT_RAW, buffer.size, name); if (cbfs_add_entry(&image, &buffer, offset, header, 0) != 0) { @@ -434,8 +437,7 @@ buffer_clone(buffer, &new_bootblock);
/* Update the location (offset) of bootblock in the region */ - *offset = convert_to_from_top_aligned(param.image_region, - buffer_size(buffer)); + *offset = convert_region_offset(buffer_size(buffer));
return 0; } @@ -519,11 +521,9 @@ /* to the cbfs file and therefore set the position */ /* the real beginning of the data. */ if (type == CBFS_COMPONENT_STAGE) - attrs->position = htonl(offset + - sizeof(struct cbfs_stage)); + attrs->position = htonl(offset - sizeof(struct cbfs_stage)); else if (type == CBFS_COMPONENT_SELF) - attrs->position = htonl(offset + - sizeof(struct cbfs_payload)); + attrs->position = htonl(offset - sizeof(struct cbfs_payload)); else attrs->position = htonl(offset); } @@ -564,8 +564,8 @@ }
if (IS_TOP_ALIGNED_ADDRESS(offset)) - offset = convert_to_from_top_aligned(param.image_region, - -offset); + offset = convert_to_from_absolute_top_aligned(param.image_region, + -offset); if (cbfs_add_entry(&image, &buffer, offset, header, len_align) != 0) { ERROR("Failed to add '%s' into ROM image.\n", filename); free(header); @@ -1336,6 +1336,16 @@ {NULL, 0, 0, 0 } };
+static uint32_t get_region_offset(long long int offset) +{ + /* If offset is not negative, no transformation required. */ + if (offset >= 0) + return offset; + + /* Calculate offset from start of region. */ + return convert_region_offset(-offset); +} + static int dispatch_command(struct command command) { if (command.accesses_region) { @@ -1373,6 +1383,17 @@ return 1; } } + + /* + * Once image region is read, input offsets can be adjusted accordingly if the + * inputs are provided as negative integers i.e. offsets from end of region. + */ + if (param.baseaddress_assigned) + param.baseaddress = get_region_offset(param.baseaddress_input); + if (param.headeroffset_assigned) + param.headeroffset = get_region_offset(param.headeroffset_input); + if (param.cbfsoffset_assigned) + param.cbfsoffset = get_region_offset(param.cbfsoffset_input); }
if (command.function()) { @@ -1592,7 +1613,7 @@ param.source_region = optarg; break; case 'b': - param.baseaddress = strtoul(optarg, &suffix, 0); + param.baseaddress_input = strtoll(optarg, &suffix, 0); if (!*optarg || (suffix && *suffix)) { ERROR("Invalid base address '%s'.\n", optarg); @@ -1643,8 +1664,7 @@ param.bootblock = optarg; break; case 'H': - param.headeroffset = strtoul( - optarg, &suffix, 0); + param.headeroffset_input = strtoll(optarg, &suffix, 0); if (!*optarg || (suffix && *suffix)) { ERROR("Invalid header offset '%s'.\n", optarg); @@ -1680,7 +1700,7 @@ param.force_pow2_pagesize = 1; break; case 'o': - param.cbfsoffset = strtoul(optarg, &suffix, 0); + param.cbfsoffset_input = strtoll(optarg, &suffix, 0); if (!*optarg || (suffix && *suffix)) { ERROR("Invalid cbfs offset '%s'.\n", optarg); @@ -1817,6 +1837,7 @@ seen_primary_cbfs = true;
param.image_region = image_regions + region; + if (dispatch_command(commands[i])) { partitioned_file_close(param.image_file); return 1;
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47829 )
Change subject: util/cbfstool: Treat region offsets differently than absolute addresses ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47829/1/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/47829/1/util/cbfstool/cbfstool.c@12... PS1, Line 128: static unsigned convert_region_offset(unsigned offset) Prefer 'unsigned int' to bare use of 'unsigned'
https://review.coreboot.org/c/coreboot/+/47829/1/util/cbfstool/cbfstool.c@12... PS1, Line 128: static unsigned convert_region_offset(unsigned offset) Prefer 'unsigned int' to bare use of 'unsigned'
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47829 )
Change subject: util/cbfstool: Treat region offsets differently than absolute addresses ......................................................................
Patch Set 1:
E: Could not add [3rdparty/amd_blobs/stoneyridge/pi/ST/FT4/AGESA.bin, 424852 bytes (414 KB)@0xdcf000]; too big? E: Failed to add '3rdparty/amd_blobs/stoneyridge/pi/ST/FT4/AGESA.bin' into ROM image. E: Failed while operating on 'FW_MAIN_A' region! E: The image will be left unmodified.
Strange that I don't see this error when using abuild locally. I will check what is different for this board.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47829 )
Change subject: util/cbfstool: Treat region offsets differently than absolute addresses ......................................................................
Patch Set 1:
Patch Set 1:
E: Could not add [3rdparty/amd_blobs/stoneyridge/pi/ST/FT4/AGESA.bin, 424852 bytes (414 KB)@0xdcf000]; too big? E: Failed to add '3rdparty/amd_blobs/stoneyridge/pi/ST/FT4/AGESA.bin' into ROM image. E: Failed while operating on 'FW_MAIN_A' region! E: The image will be left unmodified.
Strange that I don't see this error when using abuild locally. I will check what is different for this board.
Looks like the difference is jenkins selects USE_AMD_BLOBS.
Hello build bot (Jenkins), Duncan Laurie, Tim Wawrzynczak, Srinidhi N Kaushik, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47829
to look at the new patch set (#2).
Change subject: util/cbfstool: Treat region offsets differently than absolute addresses ......................................................................
util/cbfstool: Treat region offsets differently than absolute addresses
cbfstool overloads baseaddress to represent multiple things: 1. Address in SPI flash space 2. Address in host space (for x86 platforms) 3. Offset from end of region (accepted as negative number)
This was done so that the different functions that use these addresses/offsets don't need to be aware of what the value represents and can use the helper functions convert_to_from* to get the required values.
Thus, even if the user provides a negative value to represent offset from end of region, it was stored as an unsigned integer. There are special checks in convert_to_from_top_aligned which guesses if the value provided is really an offset from the end of region and converts it to an offset from start of region.
This has worked okay until now for x86 platforms because there is a single fixed decode window mapping the SPI flash to host address space. However, going forward new platforms might need to support more decode windows that are not contiguous in the host space. Thus, it is important to distinguish between offsets from end of region and addresses in host/SPI flash space and treat them separately.
As a first step towards supporting this requirement for multiple decode windows on new platforms, this change handles the negative offset provided as input in dispatch_command before the requested cbfs operation is performed.
This change adds baseaddress_input, headeroffset_input and cbfsoffset_input to struct param and converts them to offsets from start of region before storing into baseaddress, headeroffset and cbfsoffset if the inputs are negative.
In follow up changes, cbfstool will be extended to add support for multiple decode windows.
TEST=Verified using abuild with timeless option for all coreboot boards that there is no change in the resultant coreboot.rom file.
Change-Id: Ib74a7e6ed9e88fbc5489640d73bedac14872953f Signed-off-by: Furquan Shaikh furquan@google.com --- M util/cbfstool/cbfs_image.c M util/cbfstool/cbfstool.c 2 files changed, 60 insertions(+), 41 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/47829/2
Hello build bot (Jenkins), Duncan Laurie, Tim Wawrzynczak, Srinidhi N Kaushik, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47829
to look at the new patch set (#3).
Change subject: util/cbfstool: Treat region offsets differently than absolute addresses ......................................................................
util/cbfstool: Treat region offsets differently than absolute addresses
cbfstool overloads baseaddress to represent multiple things: 1. Address in SPI flash space 2. Address in host space (for x86 platforms) 3. Offset from end of region (accepted as negative number)
This was done so that the different functions that use these addresses/offsets don't need to be aware of what the value represents and can use the helper functions convert_to_from* to get the required values.
Thus, even if the user provides a negative value to represent offset from end of region, it was stored as an unsigned integer. There are special checks in convert_to_from_top_aligned which guesses if the value provided is really an offset from the end of region and converts it to an offset from start of region.
This has worked okay until now for x86 platforms because there is a single fixed decode window mapping the SPI flash to host address space. However, going forward new platforms might need to support more decode windows that are not contiguous in the host space. Thus, it is important to distinguish between offsets from end of region and addresses in host/SPI flash space and treat them separately.
As a first step towards supporting this requirement for multiple decode windows on new platforms, this change handles the negative offset provided as input in dispatch_command before the requested cbfs operation is performed.
This change adds baseaddress_input, headeroffset_input and cbfsoffset_input to struct param and converts them to offsets from start of region before storing into baseaddress, headeroffset and cbfsoffset if the inputs are negative.
In follow up changes, cbfstool will be extended to add support for multiple decode windows.
BUG=b:171534504 TEST=Verified using abuild with timeless option for all coreboot boards that there is no change in the resultant coreboot.rom file.
Change-Id: Ib74a7e6ed9e88fbc5489640d73bedac14872953f Signed-off-by: Furquan Shaikh furquan@google.com --- M util/cbfstool/cbfs_image.c M util/cbfstool/cbfstool.c 2 files changed, 60 insertions(+), 41 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/47829/3
Srinidhi N Kaushik has uploaded a new patch set (#7) to the change originally created by Furquan Shaikh. ( https://review.coreboot.org/c/coreboot/+/47829 )
Change subject: util/cbfstool: Treat region offsets differently than absolute addresses ......................................................................
util/cbfstool: Treat region offsets differently than absolute addresses
cbfstool overloads baseaddress to represent multiple things: 1. Address in SPI flash space 2. Address in host space (for x86 platforms) 3. Offset from end of region (accepted as negative number)
This was done so that the different functions that use these addresses/offsets don't need to be aware of what the value represents and can use the helper functions convert_to_from* to get the required values.
Thus, even if the user provides a negative value to represent offset from end of region, it was stored as an unsigned integer. There are special checks in convert_to_from_top_aligned which guesses if the value provided is really an offset from the end of region and converts it to an offset from start of region.
This has worked okay until now for x86 platforms because there is a single fixed decode window mapping the SPI flash to host address space. However, going forward new platforms might need to support more decode windows that are not contiguous in the host space. Thus, it is important to distinguish between offsets from end of region and addresses in host/SPI flash space and treat them separately.
As a first step towards supporting this requirement for multiple decode windows on new platforms, this change handles the negative offset provided as input in dispatch_command before the requested cbfs operation is performed.
This change adds baseaddress_input, headeroffset_input and cbfsoffset_input to struct param and converts them to offsets from start of region before storing into baseaddress, headeroffset and cbfsoffset if the inputs are negative.
In follow up changes, cbfstool will be extended to add support for multiple decode windows.
BUG=b:171534504 TEST=Verified using abuild with timeless option for all coreboot boards that there is no change in the resultant coreboot.rom file.
Change-Id: Ib74a7e6ed9e88fbc5489640d73bedac14872953f Signed-off-by: Furquan Shaikh furquan@google.com --- M util/cbfstool/cbfs_image.c M util/cbfstool/cbfstool.c 2 files changed, 60 insertions(+), 41 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/47829/7
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47829 )
Change subject: util/cbfstool: Treat region offsets differently than absolute addresses ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47829/9/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/47829/9/util/cbfstool/cbfstool.c@14... PS9, Line 149: return 0; this doesn't result in an error being propagated to the caller (it just assigns the return value) but changing that would require updating the semantics.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47829 )
Change subject: util/cbfstool: Treat region offsets differently than absolute addresses ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47829/9/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/47829/9/util/cbfstool/cbfstool.c@14... PS9, Line 149: return 0;
this doesn't result in an error being propagated to the caller (it just assigns the return value) bu […]
Yeah. I was relying on users spotting the error print above, but I think that might not be the best solution. I can rework this to return error. As you commented, this will require changing semantics at the caller site as well. Given that there are just four callers, it probably won't be too bad.
Hello build bot (Jenkins), Duncan Laurie, Tim Wawrzynczak, Srinidhi N Kaushik, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47829
to look at the new patch set (#10).
Change subject: util/cbfstool: Treat region offsets differently than absolute addresses ......................................................................
util/cbfstool: Treat region offsets differently than absolute addresses
cbfstool overloads baseaddress to represent multiple things: 1. Address in SPI flash space 2. Address in host space (for x86 platforms) 3. Offset from end of region (accepted as negative number)
This was done so that the different functions that use these addresses/offsets don't need to be aware of what the value represents and can use the helper functions convert_to_from* to get the required values.
Thus, even if the user provides a negative value to represent offset from end of region, it was stored as an unsigned integer. There are special checks in convert_to_from_top_aligned which guesses if the value provided is really an offset from the end of region and converts it to an offset from start of region.
This has worked okay until now for x86 platforms because there is a single fixed decode window mapping the SPI flash to host address space. However, going forward new platforms might need to support more decode windows that are not contiguous in the host space. Thus, it is important to distinguish between offsets from end of region and addresses in host/SPI flash space and treat them separately.
As a first step towards supporting this requirement for multiple decode windows on new platforms, this change handles the negative offset provided as input in dispatch_command before the requested cbfs operation is performed.
This change adds baseaddress_input, headeroffset_input and cbfsoffset_input to struct param and converts them to offsets from start of region before storing into baseaddress, headeroffset and cbfsoffset if the inputs are negative.
In follow up changes, cbfstool will be extended to add support for multiple decode windows.
BUG=b:171534504 TEST=Verified using abuild with timeless option for all coreboot boards that there is no change in the resultant coreboot.rom file.
Change-Id: Ib74a7e6ed9e88fbc5489640d73bedac14872953f Signed-off-by: Furquan Shaikh furquan@google.com --- M util/cbfstool/cbfs_image.c M util/cbfstool/cbfstool.c 2 files changed, 73 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/47829/10
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47829 )
Change subject: util/cbfstool: Treat region offsets differently than absolute addresses ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47829/9/util/cbfstool/cbfstool.c File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/47829/9/util/cbfstool/cbfstool.c@14... PS9, Line 149: return 0;
Yeah. […]
Done
Hello build bot (Jenkins), Duncan Laurie, Tim Wawrzynczak, Srinidhi N Kaushik, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47829
to look at the new patch set (#11).
Change subject: util/cbfstool: Treat region offsets differently than absolute addresses ......................................................................
util/cbfstool: Treat region offsets differently than absolute addresses
cbfstool overloads baseaddress to represent multiple things: 1. Address in SPI flash space 2. Address in host space (for x86 platforms) 3. Offset from end of region (accepted as negative number)
This was done so that the different functions that use these addresses/offsets don't need to be aware of what the value represents and can use the helper functions convert_to_from* to get the required values.
Thus, even if the user provides a negative value to represent offset from end of region, it was stored as an unsigned integer. There are special checks in convert_to_from_top_aligned which guesses if the value provided is really an offset from the end of region and converts it to an offset from start of region.
This has worked okay until now for x86 platforms because there is a single fixed decode window mapping the SPI flash to host address space. However, going forward new platforms might need to support more decode windows that are not contiguous in the host space. Thus, it is important to distinguish between offsets from end of region and addresses in host/SPI flash space and treat them separately.
As a first step towards supporting this requirement for multiple decode windows on new platforms, this change handles the negative offset provided as input in dispatch_command before the requested cbfs operation is performed.
This change adds baseaddress_input, headeroffset_input and cbfsoffset_input to struct param and converts them to offsets from start of region before storing into baseaddress, headeroffset and cbfsoffset if the inputs are negative.
In follow up changes, cbfstool will be extended to add support for multiple decode windows.
BUG=b:171534504 TEST=Verified using abuild with timeless option for all coreboot boards that there is no change in the resultant coreboot.rom file.
Change-Id: Ib74a7e6ed9e88fbc5489640d73bedac14872953f Signed-off-by: Furquan Shaikh furquan@google.com --- M util/cbfstool/cbfs_image.c M util/cbfstool/cbfstool.c 2 files changed, 73 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/47829/11
Hello build bot (Jenkins), Duncan Laurie, Tim Wawrzynczak, Srinidhi N Kaushik, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47829
to look at the new patch set (#12).
Change subject: util/cbfstool: Treat region offsets differently than absolute addresses ......................................................................
util/cbfstool: Treat region offsets differently than absolute addresses
cbfstool overloads baseaddress to represent multiple things: 1. Address in SPI flash space 2. Address in host space (for x86 platforms) 3. Offset from end of region (accepted as negative number)
This was done so that the different functions that use these addresses/offsets don't need to be aware of what the value represents and can use the helper functions convert_to_from* to get the required values.
Thus, even if the user provides a negative value to represent offset from end of region, it was stored as an unsigned integer. There are special checks in convert_to_from_top_aligned which guesses if the value provided is really an offset from the end of region and converts it to an offset from start of region.
This has worked okay until now for x86 platforms because there is a single fixed decode window mapping the SPI flash to host address space. However, going forward new platforms might need to support more decode windows that are not contiguous in the host space. Thus, it is important to distinguish between offsets from end of region and addresses in host/SPI flash space and treat them separately.
As a first step towards supporting this requirement for multiple decode windows on new platforms, this change handles the negative offset provided as input in dispatch_command before the requested cbfs operation is performed.
This change adds baseaddress_input, headeroffset_input and cbfsoffset_input to struct param and converts them to offsets from start of region before storing into baseaddress, headeroffset and cbfsoffset if the inputs are negative.
In follow up changes, cbfstool will be extended to add support for multiple decode windows.
BUG=b:171534504 TEST=Verified using abuild with timeless option for all coreboot boards that there is no change in the resultant coreboot.rom file.
Change-Id: Ib74a7e6ed9e88fbc5489640d73bedac14872953f Signed-off-by: Furquan Shaikh furquan@google.com --- M util/cbfstool/cbfs_image.c M util/cbfstool/cbfstool.c 2 files changed, 73 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/47829/12
Hello build bot (Jenkins), Duncan Laurie, Tim Wawrzynczak, Srinidhi N Kaushik, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47829
to look at the new patch set (#13).
Change subject: util/cbfstool: Treat region offsets differently than absolute addresses ......................................................................
util/cbfstool: Treat region offsets differently than absolute addresses
cbfstool overloads baseaddress to represent multiple things: 1. Address in SPI flash space 2. Address in host space (for x86 platforms) 3. Offset from end of region (accepted as negative number)
This was done so that the different functions that use these addresses/offsets don't need to be aware of what the value represents and can use the helper functions convert_to_from* to get the required values.
Thus, even if the user provides a negative value to represent offset from end of region, it was stored as an unsigned integer. There are special checks in convert_to_from_top_aligned which guesses if the value provided is really an offset from the end of region and converts it to an offset from start of region.
This has worked okay until now for x86 platforms because there is a single fixed decode window mapping the SPI flash to host address space. However, going forward new platforms might need to support more decode windows that are not contiguous in the host space. Thus, it is important to distinguish between offsets from end of region and addresses in host/SPI flash space and treat them separately.
As a first step towards supporting this requirement for multiple decode windows on new platforms, this change handles the negative offset provided as input in dispatch_command before the requested cbfs operation is performed.
This change adds baseaddress_input, headeroffset_input and cbfsoffset_input to struct param and converts them to offsets from start of region before storing into baseaddress, headeroffset and cbfsoffset if the inputs are negative.
In follow up changes, cbfstool will be extended to add support for multiple decode windows.
BUG=b:171534504 TEST=Verified using abuild with timeless option for all coreboot boards that there is no change in the resultant coreboot.rom file.
Change-Id: Ib74a7e6ed9e88fbc5489640d73bedac14872953f Signed-off-by: Furquan Shaikh furquan@google.com --- M util/cbfstool/cbfs_image.c M util/cbfstool/cbfstool.c 2 files changed, 73 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/47829/13
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47829 )
Change subject: util/cbfstool: Treat region offsets differently than absolute addresses ......................................................................
Patch Set 13: Code-Review+1
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47829 )
Change subject: util/cbfstool: Treat region offsets differently than absolute addresses ......................................................................
Patch Set 14: Code-Review+2
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47829 )
Change subject: util/cbfstool: Treat region offsets differently than absolute addresses ......................................................................
util/cbfstool: Treat region offsets differently than absolute addresses
cbfstool overloads baseaddress to represent multiple things: 1. Address in SPI flash space 2. Address in host space (for x86 platforms) 3. Offset from end of region (accepted as negative number)
This was done so that the different functions that use these addresses/offsets don't need to be aware of what the value represents and can use the helper functions convert_to_from* to get the required values.
Thus, even if the user provides a negative value to represent offset from end of region, it was stored as an unsigned integer. There are special checks in convert_to_from_top_aligned which guesses if the value provided is really an offset from the end of region and converts it to an offset from start of region.
This has worked okay until now for x86 platforms because there is a single fixed decode window mapping the SPI flash to host address space. However, going forward new platforms might need to support more decode windows that are not contiguous in the host space. Thus, it is important to distinguish between offsets from end of region and addresses in host/SPI flash space and treat them separately.
As a first step towards supporting this requirement for multiple decode windows on new platforms, this change handles the negative offset provided as input in dispatch_command before the requested cbfs operation is performed.
This change adds baseaddress_input, headeroffset_input and cbfsoffset_input to struct param and converts them to offsets from start of region before storing into baseaddress, headeroffset and cbfsoffset if the inputs are negative.
In follow up changes, cbfstool will be extended to add support for multiple decode windows.
BUG=b:171534504 TEST=Verified using abuild with timeless option for all coreboot boards that there is no change in the resultant coreboot.rom file.
Change-Id: Ib74a7e6ed9e88fbc5489640d73bedac14872953f Signed-off-by: Furquan Shaikh furquan@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/47829 Reviewed-by: Duncan Laurie dlaurie@chromium.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M util/cbfstool/cbfs_image.c M util/cbfstool/cbfstool.c 2 files changed, 73 insertions(+), 43 deletions(-)
Approvals: build bot (Jenkins): Verified Duncan Laurie: Looks good to me, approved Tim Wawrzynczak: Looks good to me, but someone else must approve
diff --git a/util/cbfstool/cbfs_image.c b/util/cbfstool/cbfs_image.c index 05fdc8a..7b8da42 100644 --- a/util/cbfstool/cbfs_image.c +++ b/util/cbfstool/cbfs_image.c @@ -267,14 +267,6 @@ bootblock_offset, bootblock->size, header_offset, sizeof(image->header), entries_offset);
- // Adjust legacy top-aligned address to ROM offset. - if (IS_TOP_ALIGNED_ADDRESS(entries_offset)) - entries_offset = size + (int32_t)entries_offset; - if (IS_TOP_ALIGNED_ADDRESS(bootblock_offset)) - bootblock_offset = size + (int32_t)bootblock_offset; - if (IS_TOP_ALIGNED_ADDRESS(header_offset)) - header_offset = size + (int32_t)header_offset; - DEBUG("cbfs_create_image: (real offset) bootblock=0x%x, " "header=0x%x, entries_offset=0x%x\n", bootblock_offset, header_offset, entries_offset); diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index 6936d57..d6f4e98 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -48,15 +48,30 @@ uint64_t u64val; uint32_t type; uint32_t baseaddress; + /* + * Input can be negative. It will be transformed to offset from start of region (if + * negative) and stored in baseaddress. + */ + long long int baseaddress_input; uint32_t baseaddress_assigned; uint32_t loadaddress; uint32_t headeroffset; + /* + * Input can be negative. It will be transformed to offset from start of region (if + * negative) and stored in baseaddress. + */ + long long int headeroffset_input; uint32_t headeroffset_assigned; uint32_t entrypoint; uint32_t size; uint32_t alignment; uint32_t pagesize; uint32_t cbfsoffset; + /* + * Input can be negative. It will be transformed to corresponding region offset (if + * negative) and stored in baseaddress. + */ + long long int cbfsoffset_input; uint32_t cbfsoffset_assigned; uint32_t arch; uint32_t padding; @@ -103,8 +118,7 @@
/* * Converts between offsets from the start of the specified image region and - * "top-aligned" offsets from the top of the entire boot media. See comment - * below for convert_to_from_top_aligned() about forming addresses. + * "top-aligned" offsets from the top of the entire boot media. */ static unsigned convert_to_from_absolute_top_aligned( const struct buffer *region, unsigned offset) @@ -117,25 +131,27 @@ }
/* - * Converts between offsets from the start of the specified image region and - * "top-aligned" offsets from the top of the image region. Works in either - * direction: pass in one type of offset and receive the other type. - * N.B. A top-aligned offset is always a positive number, and should not be - * confused with a top-aligned *address*, which is its arithmetic inverse. */ -static unsigned convert_to_from_top_aligned(const struct buffer *region, - unsigned offset) + * This function takes offset value which represents the offset from one end of the region and + * converts it to offset from the other end of the region. offset is expected to be positive. + */ +static int convert_region_offset(unsigned int offset, uint32_t *region_offset) { - assert(region); + size_t size;
- /* Cover the situation where a negative base address is given by the - * user. Callers of this function negate it, so it'll be a positive - * number smaller than the region. - */ - if ((offset > 0) && (offset < region->size)) { - return region->size - offset; + if (param.size) { + size = param.size; + } else { + assert(param.image_region); + size = param.image_region->size; }
- return convert_to_from_absolute_top_aligned(region, offset); + if (size < offset) { + ERROR("Cannot convert region offset (size=0x%zx, offset=0x%x)\n", size, offset); + return 1; + } + + *region_offset = size - offset; + return 0; }
static int do_cbfs_locate(int32_t *cbfs_addr, size_t metadata_size, @@ -243,10 +259,6 @@ goto done; }
- if (IS_TOP_ALIGNED_ADDRESS(offset)) - offset = convert_to_from_top_aligned(param.image_region, - -offset); - header = cbfs_create_file_header(CBFS_TYPE_RAW, buffer.size, name); if (cbfs_add_entry(&image, &buffer, offset, header, 0) != 0) { @@ -435,10 +447,7 @@ buffer_clone(buffer, &new_bootblock);
/* Update the location (offset) of bootblock in the region */ - *offset = convert_to_from_top_aligned(param.image_region, - buffer_size(buffer)); - - return 0; + return convert_region_offset(buffer_size(buffer), offset); }
static int cbfs_add_component(const char *filename, @@ -520,11 +529,9 @@ /* to the cbfs file and therefore set the position */ /* the real beginning of the data. */ if (type == CBFS_TYPE_STAGE) - attrs->position = htonl(offset + - sizeof(struct cbfs_stage)); + attrs->position = htonl(offset - sizeof(struct cbfs_stage)); else if (type == CBFS_TYPE_SELF) - attrs->position = htonl(offset + - sizeof(struct cbfs_payload)); + attrs->position = htonl(offset - sizeof(struct cbfs_payload)); else attrs->position = htonl(offset); } @@ -565,8 +572,7 @@ }
if (IS_TOP_ALIGNED_ADDRESS(offset)) - offset = convert_to_from_top_aligned(param.image_region, - -offset); + offset = convert_to_from_absolute_top_aligned(param.image_region, -offset); if (cbfs_add_entry(&image, &buffer, offset, header, len_align) != 0) { ERROR("Failed to add '%s' into ROM image.\n", filename); free(header); @@ -1345,6 +1351,32 @@ {NULL, 0, 0, 0 } };
+static int get_region_offset(long long int offset, uint32_t *region_offset) +{ + /* If offset is not negative, no transformation required. */ + if (offset >= 0) { + *region_offset = offset; + return 0; + } + + /* Calculate offset from start of region. */ + return convert_region_offset(-offset, region_offset); +} + +static int calculate_region_offsets(void) +{ + int ret = 0; + + if (param.baseaddress_assigned) + ret |= get_region_offset(param.baseaddress_input, ¶m.baseaddress); + if (param.headeroffset_assigned) + ret |= get_region_offset(param.headeroffset_input, ¶m.headeroffset); + if (param.cbfsoffset_assigned) + ret |= get_region_offset(param.cbfsoffset_input, ¶m.cbfsoffset); + + return ret; +} + static int dispatch_command(struct command command) { if (command.accesses_region) { @@ -1382,6 +1414,13 @@ return 1; } } + + /* + * Once image region is read, input offsets can be adjusted accordingly if the + * inputs are provided as negative integers i.e. offsets from end of region. + */ + if (calculate_region_offsets()) + return 1; }
if (command.function()) { @@ -1598,7 +1637,7 @@ param.source_region = optarg; break; case 'b': - param.baseaddress = strtoul(optarg, &suffix, 0); + param.baseaddress_input = strtoll(optarg, &suffix, 0); if (!*optarg || (suffix && *suffix)) { ERROR("Invalid base address '%s'.\n", optarg); @@ -1649,8 +1688,7 @@ param.bootblock = optarg; break; case 'H': - param.headeroffset = strtoul( - optarg, &suffix, 0); + param.headeroffset_input = strtoll(optarg, &suffix, 0); if (!*optarg || (suffix && *suffix)) { ERROR("Invalid header offset '%s'.\n", optarg); @@ -1686,7 +1724,7 @@ param.force_pow2_pagesize = 1; break; case 'o': - param.cbfsoffset = strtoul(optarg, &suffix, 0); + param.cbfsoffset_input = strtoll(optarg, &suffix, 0); if (!*optarg || (suffix && *suffix)) { ERROR("Invalid cbfs offset '%s'.\n", optarg);
-
build bot (Jenkins) (Code Review) -
Duncan Laurie (Code Review)
-
Furquan Shaikh (Code Review)
-
Srinidhi N Kaushik (Code Review)
-
Tim Wawrzynczak (Code Review)