Change in coreboot[master]: cbfstool: Do host space address conversion earlier when adding files

[Julius Werner (Code Review)]

Attention is currently required from: Patrick Georgi.
Hello Patrick Georgi,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/60018

to review the following change.

Change subject: cbfstool: Do host space address conversion earlier when adding files
......................................................................

cbfstool: Do host space address conversion earlier when adding files

A simple code simplification where we can pull the call to
convert_addr_space() in cbfs_add_component() earlier, so that we can
avoid having to duplicate it in cbfstool_convert_fsp(). Host space
addresses in this function can only occur if the user manually provided
them via the --base-address flag -- all other functions (e.g.
do_cbfs_locate()) already work in the flash address space anyway. This
should also fix a tiny issue where --gen-attribute might have previously
encoded the address as given in CBFS -- it probably makes more sense to
always have it store a consistent format (i.e. always flash address).

Also revert the unnecessary check for --base-address in
add_topswap_bootblock(). On closer inspection, the function actually
doesn't use the passed in *offset at all and uses it purely as an
out-parameter. So while our current Makefile does pass --base-address
when adding the bootblock, it actually has no effect and is redundant
for the topswap case.

Signed-off-by: Julius Werner 
Change-Id: Idf4721c5b0700789ddb81c1618d740b3e7f486cb
---
M util/cbfstool/cbfstool.c
1 file changed, 10 insertions(+), 22 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/60018/1

diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c
index 87dcbb4..0f1bc9e 100644
--- a/util/cbfstool/cbfstool.c
+++ b/util/cbfstool/cbfstool.c
@@ -779,11 +779,6 @@
 {
    size_t bb_buf_size = buffer_size(buffer);

-	if (!param.baseaddress_assigned) {
-		ERROR("--topswap-size must also have --base-address\n");
-		return 1;
-	}
-
    if (bb_buf_size > param.topswap_size) {
    	ERROR("Bootblock bigger than the topswap boundary\n");
    	ERROR("size = %zd, ts = %d\n", bb_buf_size,
@@ -841,6 +836,7 @@
    uint32_t offset = param.baseaddress_assigned ? param.baseaddress : 0;
    size_t len_align = 0;

+
    if (param.alignment && param.baseaddress_assigned) {
    	ERROR("Cannot specify both alignment and base address\n");
    	return 1;
@@ -909,6 +905,10 @@
    	if (add_topswap_bootblock(&buffer, &offset))
    		goto error;

+	/* With --base-address we allow host space addresses -- if so, convert it here. */
+	if (IS_HOST_SPACE_ADDRESS(offset))
+		offset = convert_addr_space(param.image_region, offset);
+
    if (convert && convert(&buffer, &offset, header) != 0) {
    	ERROR("Failed to parse file '%s'.\n", filename);
    	goto error;
@@ -975,9 +975,6 @@
    		goto error;
    }

-	if (IS_HOST_SPACE_ADDRESS(offset))
-		offset = convert_addr_space(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);
    	goto error;
@@ -1079,10 +1076,7 @@
    		if (do_cbfs_locate(offset, 0))
    			return -1;
    	}
-		if (!IS_HOST_SPACE_ADDRESS(*offset))
-			address = convert_addr_space(param.image_region, *offset);
-		else
-			address = *offset;
+		address = *offset;
    } else {
    	if (param.baseaddress_assigned == 0) {
    		INFO("Honoring pre-linked FSP module, no relocation.\n");
@@ -1143,16 +1137,10 @@
    	return -1;

if (param.stage_xip) {
-		/*
-		 * Ensure the address is a memory mapped one. This assumes
-		 * x86 semantics about the boot media being directly mapped
-		 * below 4GiB in the CPU address space.
-		 **/
-		*offset = convert_addr_space(param.image_region, *offset);
-
-		ret = parse_elf_to_xip_stage(buffer, &output, *offset,
-					     param.ignore_section,
-					     stageheader);
+		uint32_t host_space_address = convert_addr_space(param.image_region, *offset);
+		assert(IS_HOST_SPACE_ADDRESS(host_space_address));
+		ret = parse_elf_to_xip_stage(buffer, &output, host_space_address,
+					     param.ignore_section, stageheader);
    } else {
    	ret = parse_elf_to_stage(buffer, &output, param.ignore_section,
    				 stageheader);