Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33300 )
Change subject: [RFC and WIP]Add an option to align FMAP regions ......................................................................
Patch Set 1:
(5 comments)
https://review.coreboot.org/#/c/33300/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33300/1//COMMIT_MSG@15 PS1, Line 15: such configuration presents itself Should definitely error out, I think. I think we can already have the parser take care of this if we design the syntax well.
https://review.coreboot.org/#/c/33300/1//COMMIT_MSG@16 PS1, Line 16: - '~' is used. Is that the 'best' symbol So this would be written
SECTION 32K ~4K (CBFS)
? Yeah... not super readable but I also can't think of something significantly better. But I think alignment should come before the size and be written more similar to explicit base offsets instead, because those two are sort of the same thing and mutually exclusive.
So maybe one of these?
SECTION~4K 32K (CBFS) SECTION(4K) 32K (CBFS) SECTION[4K] 32K (CBFS) SECTION.4K 32K (CBFS) SECTION|4K 32K (CBFS) SECTION/4K 32K (CBFS) SECTION$4K 32K (CBFS)
I think SECTION/4K might be the best option, because the slash invokes the idea of division and alignment is essentially "must be divisible by"? (I guess SECTION%4K could work with the same argument but looks weirder I think.)
https://review.coreboot.org/#/c/33300/1/util/cbfstool/fmd.c File util/cbfstool/fmd.c:
https://review.coreboot.org/#/c/33300/1/util/cbfstool/fmd.c@132 PS1, Line 132: cur->offset = end_watermark -= cur->size; I think this also needs to take alignment into account.
https://review.coreboot.org/#/c/33300/1/util/cbfstool/fmd.c@218 PS1, Line 218: cur_section->offset = watermark; Shouldn't you do the alignment adjustment right here, so that the actual (aligned) offset is used to update the watermark in line 249?
https://review.coreboot.org/#/c/33300/1/util/cbfstool/fmd.c@252 PS1, Line 252: cur_section->offset = (cur_section->offset + cur_section->alignment - 1) Just use ALIGN_UP from <commonlib/helpers.h>