[coreboot-gerrit] [XL] Change in coreboot[master]: util: cbfstool: Add '%<alignment>' in flashmap descriptor

[Julius Werner (Code Review)]

Attention is currently required from: Aaron Durbin, Furquan Shaikh, Hung-Te Lin.

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37262?usp=email )

Change subject: util: cbfstool: Add '%<alignment>' in flashmap descriptor
......................................................................

Patch Set 13:

(5 comments)

File util/cbfstool/default.fmd:

https://review.coreboot.org/c/coreboot/+/37262/comment/0eaf611f_9d79c430 :
PS13, Line 15:  
There should be no space here, right? (I mean, I guess it may still work if there is, but I think it looks better without and that's how the documentation suggests it.)

https://review.coreboot.org/c/coreboot/+/37262/comment/4aecd6ab_72074215 :
PS13, Line 18:  %0
Do we really want to not enforce alignment for CBFS? After all we do have the -a flag for cbfstool which is also regularly used for various things, and that doesn't make sense if the parent section is not aligned. I think making CBFS default to 4K as well makes sense. (Does this not build if you do that? If so, can we fix those cases?)

File util/cbfstool/fmap_from_fmd.c:

https://review.coreboot.org/c/coreboot/+/37262/comment/70472dc8_e263a115 :
PS13, Line 29: WARN
I think this should be a hard error, but I guess the plan is to make it a warning first and then change to error after some adjustment period?

File util/cbfstool/fmd_parser.y:

https://review.coreboot.org/c/coreboot/+/37262/comment/d96e1a89_cd333625 :
PS13, Line 103: 	| region_flag ',' region_flags { $$.v = $1.v | $3.v; };
This doesn't look wrong, but unrelated? Also, does the previous rule (`region_flag region_flags`) even make sense (would that implicitly be space-separated or does it just mean you're literally supposed to write `CBFSPRESERVE`, which then wouldn't work because the tokenizer sees it as one word)? If not, shouldn't you replace it rather than add another alternative?

File util/cbfstool/fmd_scanner.l:

https://review.coreboot.org/c/coreboot/+/37262/comment/4f6ae21c_1be01f61 :
PS13, Line 31: .                           return *yytext;
I don't really understand lex at all, but if you want to add `,` as a new valid token for separating flags, don't you need to mention it somewhere here as well? I'm confused why the percent sign goes here but the comma doesn't, since they're basically used in the same manner.