Change in coreboot[master]: util/cbfstool: Add support for filling ID region using FMAP

[Furquan Shaikh (Code Review)]

Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40376 )

Change subject: util/cbfstool: Add support for filling ID region using FMAP
......................................................................

util/cbfstool: Add support for filling ID region using FMAP

This change adds a new command add-id which takes as input the
following parameters:
1. ID_FMAP_REGION: Name of the FMAP region used for storing
ID (coreboot build details)
2. VERSION: Coreboot version provided by the build system.
3. VENDOR: Vendor name provided by the build system.
4. PART_NUM: Part number provided by the build system.

Using the above parameters, this new command finds the fmap region
using ID_FMAP_REGION and copies version, vendor and part number
strings into the ID region.

This allows us to get rid of all the id.ld and id.S files that are
currently added in coreboot to supply build details into the bootblock
or decompressor binary.

This command makes use of non-ASCII longopts since we are running out
of good opt characters to use.

Signed-off-by: Furquan Shaikh 
Change-Id: Ic5c55a49025508ff1f27c6f48d8e420f19d88ec2
---
M util/cbfstool/cbfstool.c
1 file changed, 60 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/40376/1

diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c
index f15c65b..224672c 100644
--- a/util/cbfstool/cbfstool.c
+++ b/util/cbfstool/cbfstool.c
@@ -93,6 +93,9 @@
    char *initrd;
    char *cmdline;
    int force;
+	const char *id_version;
+	const char *id_vendor;
+	const char *id_part_num;
 } param = {
    /* All variables not listed are initialized as zero. */
    .arch = CBFS_ARCHITECTURE_UNKNOWN,
@@ -1270,6 +1273,44 @@
    return result;
 }

+static int id_str_write(struct buffer *buff, const char *str)
+{
+	size_t len = strlen(str) + 1;
+	if (buffer_size(buff) < len) {
+		ERROR("Not enough space for string in %s\n", param.region_name);
+		return 1;
+	}
+
+	strcpy(buffer_get(buff), str);
+	buffer_seek(buff, len);
+	return 0;
+}
+
+static int cbfs_add_id(void)
+{
+	if (!param.id_version || !param.id_vendor || !param.id_part_num) {
+		ERROR("All ID params (version, vendor and part_num) are mandatory\n");
+		return 1;
+	}
+
+	printf("FUR: %s %s %s\n", param.id_version, param.id_vendor, param.id_part_num);
+
+	struct buffer id_buffer;
+
+	buffer_clone(&id_buffer, param.image_region);
+
+	if (id_str_write(&id_buffer, param.id_version))
+		return 1;
+
+	if (id_str_write(&id_buffer, param.id_vendor))
+		return 1;
+
+	if (id_str_write(&id_buffer, param.id_part_num))
+		return 1;
+
+	return 0;
+}
+
 static const struct command commands[] = {
    {"add", "H:r:f:n:t:c:b:a:p:yvA:j:gh?", cbfs_add, true, true},
    {"add-flat-binary", "H:r:f:n:l:e:c:b:p:vA:gh?", cbfs_add_flat_binary,
@@ -1280,6 +1321,7 @@
    			true, true},
    {"add-int", "H:r:i:n:b:vgh?", cbfs_add_integer, true, true},
    {"add-master-header", "H:r:vh?j:", cbfs_add_master_header, true, true},
+	{"add-id", "r:h?", cbfs_add_id, true, true},
    {"compact", "r:h?", cbfs_compact, true, true},
    {"copy", "r:R:h?", cbfs_copy, true, true},
    {"create", "M:r:s:B:b:H:o:m:vh?", cbfs_create, true, true},
@@ -1297,6 +1339,9 @@
    /* begin after ASCII characters */
    LONGOPT_START = 256,
    LONGOPT_IBB = LONGOPT_START,
+	LONGOPT_ID_VERSION,
+	LONGOPT_ID_VENDOR,
+	LONGOPT_ID_PART_NUM,
    LONGOPT_END,
 };

@@ -1339,6 +1384,9 @@
    {"mach-parseable",no_argument,       0, 'k' },
    {"unprocessed",   no_argument,       0, 'U' },
    {"ibb",           no_argument,       0, LONGOPT_IBB },
+	{"id-version",    required_argument, 0, LONGOPT_ID_VERSION },
+	{"id-vendor",     required_argument, 0, LONGOPT_ID_VENDOR },
+	{"id-part-num",   required_argument, 0, LONGOPT_ID_PART_NUM },
    {NULL,            0,                 0,  0  }
 };

@@ -1433,6 +1481,9 @@
         " add-master-header [-r image,regions] \                   \n"
         "        [-j topswap-size] (Intel CPUs only)                  "
    		"Add a legacy CBFS master header\n"
+	     " add-id -r ID_FMAP_REGION --id-version VERSION \\n"
+	     "        --id-vendor VENDOR --id-part-num PART_NUM            "
+			"Add build details to ID FMAP region\n"
         " remove [-r image,regions] -n NAME                           "
    		"Remove a component\n"
         " compact -r image,regions                                    "
@@ -1752,6 +1803,15 @@
    		case LONGOPT_IBB:
    			param.ibb = true;
    			break;
+			case LONGOPT_ID_VERSION:
+				param.id_version = optarg;
+				break;
+			case LONGOPT_ID_VENDOR:
+				param.id_vendor = optarg;
+				break;
+			case LONGOPT_ID_PART_NUM:
+				param.id_part_num = optarg;
+				break;
    		case 'h':
    		case '?':
    			usage(argv[0]);

[Furquan Shaikh (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#2).

Change subject: util/cbfstool: Add support for filling ID region using FMAP
......................................................................

util/cbfstool: Add support for filling ID region using FMAP

This change adds a new command add-id which takes as input the
following parameters:
1. ID_FMAP_REGION: Name of the FMAP region used for storing
ID (coreboot build details)
2. VERSION: coreboot version provided by the build system.
3. VENDOR: Vendor name provided by the build system.
4. PART_NUM: Part number provided by the build system.

Using the above parameters, this new command finds the fmap region
using ID_FMAP_REGION and copies version, vendor and part number
strings into the ID region.

This allows us to get rid of all the id.ld and id.S files that are
currently added in coreboot to supply build details into the bootblock
or decompressor binary.

This command makes use of non-ASCII longopts since we are running out
of good opt characters to use.

Signed-off-by: Furquan Shaikh 
Change-Id: Ic5c55a49025508ff1f27c6f48d8e420f19d88ec2
---
M util/cbfstool/cbfstool.c
1 file changed, 58 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/40376/2

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40376 )

Change subject: util/cbfstool: Add support for filling ID region using FMAP
......................................................................

Patch Set 2:

1. What is the motivation for this? Are we just trying to save a few bytes of bootblock space? (Otherwise, the extra hassle of having to put yet another FMAP section everywhere seems more like a downside to me...)

2. Why does this have to be done by cbfstool? Wouldn't it be easier to just put this as a struct in a C file, compile it, objcopy to binary and use cbfstool write?

(And if the goal just is to save bootblock space we could also have that C file and just put it as a separate file into CBFS -- we already have cbfs-files-processor-struct to make that easy. FMAP sections really only need to be used when alignment to erase blocks is important, I don't see how that would apply here.)

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40376 )

Change subject: util/cbfstool: Add support for filling ID region using FMAP
......................................................................

Patch Set 2:
> Patch Set 2:
>
> What is the motivation for this? Are we just trying to save a few bytes of bootblock space? (Otherwise, the extra hassle of having to put yet another FMAP section everywhere seems more like a downside to me...)
>
> Why does this have to be done by cbfstool? Wouldn't it be easier to just put this as a struct in a C file, compile it, objcopy to binary and use cbfstool write?
>
> (And if the goal just is to save bootblock space we could also have that C file and just put it as a separate file into CBFS -- we already have cbfs-files-processor-struct to make that easy. FMAP sections really only need to be used when alignment to erase blocks is important, I don't see how that would apply here.)

The motivation here is not to save space. The number of bytes that will be saved is just too less to warrant the effort.

I started looking at the id because of the recent changes that are going in for supporting the Picasso platform: https://review.coreboot.org/c/coreboot/+/35035/21/src/arch/x86/memlayout.ld#.... I don't think this change functionally helps that platform, but it helps keep the code clean w.r.t. different pieces being added. Do these details really need to be added to bootblock/decompressor regions?

Why not CBFS? I am not sure how/if tools use this information. Adding to CBFS would require tools to either understand cbfs walking or use some other marker to identify where the details are present. Adding FMAP entry allows the tools to use FMAP as the source of truth to locate the right offset.

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40376 )

Change subject: util/cbfstool: Add support for filling ID region using FMAP
......................................................................

Patch Set 2:
> I started looking at the id because of the recent changes that are going in for supporting the Picasso platform: https://review.coreboot.org/c/coreboot/+/35035/21/src/arch/x86/memlayout.ld#.... I don't think this change functionally helps that platform, but it helps keep the code clean w.r.t. different pieces being added. Do these details really need to be added to bootblock/decompressor regions?

I think they were there mostly because that guarantees they're not going to be compressed, but I agree, they don't really need to be embedded in any stage.
> Why not CBFS? I am not sure how/if tools use this information. Adding to CBFS would require tools to either understand cbfs walking or use some other marker to identify where the details are present. Adding FMAP entry allows the tools to use FMAP as the source of truth to locate the right offset.

I'm pretty sure no tools use this at all right now. I do think CBFS would be the more appropriate place to put this (and would make the implementation easier because we already have that mechanism to add C structs as files). In fact there are other things that would probably be worth pulling into CBFS in the long term (e.g. the vboot FWIDs or the GBB), but at least there we have existing tooling that would need to be updated blocking that. For this thing where you are changing where it lives anyway, I think putting it into CBFS would make more sense. FMAP is fundamentally not a great/scalable way to organize things and I think things should only need to be FMAP sections if they have a good reason not to be CBFS files.

For things that might want to read this in the future, I don't see why we would assume they're more likely to have an FMAP parser than a CBFS parser. (In fact, I bet more people would have cbfstool than dump_fmap installed. Also, if we really care about people being able to access this easily without complicated parsers, the best approach would probably be to put a big, unique magic number in front and then store it uncompressed in CBFS... then you can just scan the whole image for that number if necessary.)

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40376 )

Change subject: util/cbfstool: Add support for filling ID region using FMAP
......................................................................

Patch Set 2:
> Patch Set 2:
> > I started looking at the id because of the recent changes that are going in for supporting the Picasso platform: https://review.coreboot.org/c/coreboot/+/35035/21/src/arch/x86/memlayout.ld#.... I don't think this change functionally helps that platform, but it helps keep the code clean w.r.t. different pieces being added. Do these details really need to be added to bootblock/decompressor regions?
>
> I think they were there mostly because that guarantees they're not going to be compressed, but I agree, they don't really need to be embedded in any stage.
> > Why not CBFS? I am not sure how/if tools use this information. Adding to CBFS would require tools to either understand cbfs walking or use some other marker to identify where the details are present. Adding FMAP entry allows the tools to use FMAP as the source of truth to locate the right offset.
>
> I'm pretty sure no tools use this at all right now. I do think CBFS would be the more appropriate place to put this (and would make the implementation easier because we already have that mechanism to add C structs as files). In fact there are other things that would probably be worth pulling into CBFS in the long term (e.g. the vboot FWIDs or the GBB), but at least there we have existing tooling that would need to be updated blocking that. For this thing where you are changing where it lives anyway, I think putting it into CBFS would make more sense. FMAP is fundamentally not a great/scalable way to organize things and I think things should only need to be FMAP sections if they have a good reason not to be CBFS files.
>
> For things that might want to read this in the future, I don't see why we would assume they're more likely to have an FMAP parser than a CBFS parser. (In fact, I bet more people would have cbfstool than dump_fmap installed. Also, if we really care about people being able to access this easily without complicated parsers, the best approach would probably be to put a big, unique magic number in front and then store it uncompressed in CBFS... then you can just scan the whole image for that number if necessary.)

I am thinking of tools like flashrom which already understand FMAP and can make use of it to get to this information. I think the support in flashrom for reading this region is broken, but I don't think we want to add a cbfs parser to flashrom for getting to this region.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40376 )

Change subject: util/cbfstool: Add support for filling ID region using FMAP
......................................................................

Patch Set 2: Code-Review+1

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40376 )

Change subject: util/cbfstool: Add support for filling ID region using FMAP
......................................................................

Patch Set 2: Code-Review-1

(Don't think this needs to be here, see CB:40377 discussion. Even if we wanted to have it in the FMAP, I still think it would be better to just create the blob in C and cbfstool write it rather than add all this custom functionality to CBFS just for this.)

[Furquan Shaikh (Code Review)]

Furquan Shaikh has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/40376 )

Change subject: util/cbfstool: Add support for filling ID region using FMAP
......................................................................

Abandoned

Haven't done anything in 1+ year