Attention is currently required from: Raul Rangel, Tim Wawrzynczak. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59877 )
Change subject: cbfstool: Fix offset calculation for aligned files ......................................................................
Patch Set 2:
(2 comments)
File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/59877/comment/fc1944db_cc9a909b PS2, Line 1085: *offset;
Do we also need to convert the address here?
I don't really understand this x86-specific address space stuff well, so I hope someone can correct me if I'm wrong... but as far as I understand, there's only the "host" address space and the "flash" address space... so if *offset is not in the host address space that means it already is in the flash address space and good to go as is. I think in practice, do_cbfs_locate() always generates offsets in the flash address space anyway, so this is only here for convenience with the --base-address parameter so people can pass -B 0xfffff000 instead of -B 0x7ff000.
Anyway, I was just trying to preserve the previous behavior here which I think this does.
https://review.coreboot.org/c/coreboot/+/59877/comment/af5466c3_8fe46c32 PS2, Line 1299: param.filename, : param.name, : param.headeroffset,
If we are just using the param global, do we even need these parameters?
Uff... yes... no... I don't know. I mean, in general passing everything between functions in a global is an antipattern and should be avoided. But on the other hand, do_cbfs_locate() is already using most of the stuff from `param` so it seems silly not to do it for this as well. And if you both pass something by value and have it in the global struct, that's also dangerous because then they may go out of sync (e.g. here, before this patch, param.type is actually not set to SELF for cbfstool add-payload and if something checks it it would give the wrong value).
Maybe it makes sense to get rid of the other three parameters here and just use `param` everywhere for consistency. But I don't directly need that for what I'm trying to do here so I don't want to do it in this patch... like Patrick said, it's already getting a bit too big.