Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33300
Change subject: [RFC and WIP]Add an option to align FMAP regions ......................................................................
[RFC and WIP]Add an option to align FMAP regions
This eases writing FMD files when there are alignment requirement on some regions (e.g. MRC_VAR_CACHE).
Some questions: - Do we want to error out on options that have a base defined but that does not follow its alignment or do we want to fix the alignment if such configuration presents itself - '~' is used. Is that the 'best' symbol
Change-Id: If6e01112f0c98f45876bcc7012f195f5aa693efd Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M util/cbfstool/fmd.c M util/cbfstool/fmd.h M util/cbfstool/fmd_parser.y M util/cbfstool/fmd_scanner.l 4 files changed, 22 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/33300/1
diff --git a/util/cbfstool/fmd.c b/util/cbfstool/fmd.c index 7a289d7..d39ec20 100644 --- a/util/cbfstool/fmd.c +++ b/util/cbfstool/fmd.c @@ -248,6 +248,10 @@ } else if (!first_incomplete_it) { watermark = cur_section->offset + cur_section->size; } + if (cur_section->alignment_known) { + cur_section->offset = (cur_section->offset + cur_section->alignment - 1) + & ~(cur_section->alignment); + } }
if (first_incomplete_it && diff --git a/util/cbfstool/fmd.h b/util/cbfstool/fmd.h index 90e6d6e..91ae2fd 100644 --- a/util/cbfstool/fmd.h +++ b/util/cbfstool/fmd.h @@ -53,6 +53,8 @@ bool size_known; /** It is an error to read this field unless size_known is set. */ unsigned size; + bool alignment_known; + unsigned alignment; size_t list_len; union flashmap_flags flags; /** It is an error to dereference this array if list_len is 0. */ diff --git a/util/cbfstool/fmd_parser.y b/util/cbfstool/fmd_parser.y index 3ba710c..e7e1caf 100644 --- a/util/cbfstool/fmd_parser.y +++ b/util/cbfstool/fmd_parser.y @@ -52,7 +52,8 @@
struct flashmap_descriptor *parse_descriptor( char *name, union flashmap_flags flags, struct unsigned_option offset, - struct unsigned_option size, struct descriptor_list children); + struct unsigned_option size, struct unsigned_option alignment, + struct descriptor_list children); void yyerror(const char *s); }
@@ -71,6 +72,8 @@ %type <maybe_intval> region_offset %type <maybe_intval> region_size_opt %type <maybe_intval> region_size +%type <maybe_intval> region_alignment_opt +%type <maybe_intval> region_alignment %type <region_listhdr> region_list_opt %type <region_listhdr> region_list %type <region_listhdr> region_list_entries @@ -80,13 +83,14 @@ flash_chip: region_name region_offset_opt region_size region_list { union flashmap_flags flags = { .v=0 }; - if (!(res = parse_descriptor($1, flags, $2, $3, $4))) + struct unsigned_option alignment = { .val_known = false }; + if (!(res = parse_descriptor($1, flags, $2, $3, alignment, $4))) YYABORT; }; flash_region: region_name region_flags_opt region_offset_opt region_size_opt - region_list_opt + region_alignment_opt region_list_opt { - struct flashmap_descriptor *node = parse_descriptor($1, $2, $3, $4, $5); + struct flashmap_descriptor *node = parse_descriptor($1, $2, $3, $4, $5, $6); if (!node) YYABORT;
@@ -115,6 +119,9 @@ region_size_opt: { $$ = (struct unsigned_option){false, 0}; } | region_size; region_size: INTEGER { $$ = (struct unsigned_option){true, $1}; }; +region_alignment_opt: { $$ = (struct unsigned_option){false, 0}; } + | region_alignment; +region_alignment: '~' INTEGER { $$ = (struct unsigned_option){true, $2}; }; region_list_opt: { $$ = (struct descriptor_list) @@ -152,7 +159,8 @@
struct flashmap_descriptor *parse_descriptor( char *name, union flashmap_flags flags, struct unsigned_option offset, - struct unsigned_option size, struct descriptor_list children) + struct unsigned_option size, struct unsigned_option alignment, + struct descriptor_list children) { struct flashmap_descriptor *region = malloc(sizeof(*region)); if (!region) { @@ -165,6 +173,8 @@ region->offset = offset.val; region->size_known = size.val_known; region->size = size.val; + region->alignment_known = alignment.val_known; + region->alignment = alignment.val; region->list_len = children.len; if (region->list_len) { region->list = malloc(region->list_len * sizeof(*region->list)); diff --git a/util/cbfstool/fmd_scanner.l b/util/cbfstool/fmd_scanner.l index be9a5de..4c7d322 100644 --- a/util/cbfstool/fmd_scanner.l +++ b/util/cbfstool/fmd_scanner.l @@ -39,7 +39,7 @@ [1-9][0-9]*{MULTIPLIER}? return parse_integer(yytext, 10); 0[0-9]+{MULTIPLIER}? return OCTAL; 0[xX][0-9a-fA-F]+{MULTIPLIER}? return parse_integer(yytext + 2, 16); -[^#@{}()[:space:]]* return copy_string(yytext); +[^#@~{}()[:space:]]* return copy_string(yytext); . return *yytext;
%%
build bot (Jenkins) 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:
(2 comments)
https://review.coreboot.org/#/c/33300/1/util/cbfstool/fmd.h File util/cbfstool/fmd.h:
https://review.coreboot.org/#/c/33300/1/util/cbfstool/fmd.h@57 PS1, Line 57: unsigned alignment; Prefer 'unsigned int' to bare use of 'unsigned'
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@252 PS1, Line 252: cur_section->offset = (cur_section->offset + cur_section->alignment - 1) line over 80 characters
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>
Arthur Heymans 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:
(2 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. […]
so there are 3 options I see: - have both a base and alignment and have to code fixup the base if it's not aligned (probably bad idea) - have both a base and alignment and error out if base is not aligned but continue if it is (works as a check) - make base and alignment mutually exclusive (probably cleanest)
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 […]
the flags are now places right after the SECTION name, so it looks like SECTION(<flags>)@base size ~alignment at the moment. Do we want to change that? I personally like how it looks in your proposal with the flags as last 'argument'. '/' sounds ok
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:
(2 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
so there are 3 options I see: […]
I think they should be mutually exclusive. That makes the syntax easiest and I don't think there's any use case for having both together. When you put an explicit offset, that should be the offset you get.
https://review.coreboot.org/#/c/33300/1//COMMIT_MSG@16 PS1, Line 16: - '~' is used. Is that the 'best' symbol
the flags are now places right after the SECTION name, so it looks like SECTION(<flags>)@base size ~ […]
Oh... no, sorry, that was just me misremembering the format, I don't think we need to change the flags format. But I think the alignment should be written in place of the explicit offset, wherever that is. So the new format should be
SECTION(CBFS)@0x10000 32K # for explicit offsets SECTION(CBFS)/4K 32K # for auto-placed with alignment
Arthur Heymans has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/33300 )
Change subject: [RFC and WIP]Add an option to align FMAP regions ......................................................................
Abandoned
If you align a region and a region below it has a fixed offset, there might be overlap. It's possible to deal with that, but it's probably better to deal with that when generating the fmd file instead of in the fmd parser.
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:
Forgot this patch existed. I remember discussing this with someone else more recently too, don't quite remember with whom... maybe +Hung-Te? I think the solution we discussed there was rather to add another attribute "ALIGNED" that doesn't auto-align the section but instead just errors out when the section isn't aligned. So you would write
RW_VPD(PRESERVE,ALIGNED) 32K
and then if at any point you change something else in the file such that this ends up at a non-aligned boundary, you'll get a build-time error. The assumption there is that the only alignment we'd care about is 4K for the erase block.
Arthur Heymans 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:
Patch Set 1:
Forgot this patch existed. I remember discussing this with someone else more recently too, don't quite remember with whom... maybe +Hung-Te? I think the solution we discussed there was rather to add another attribute "ALIGNED" that doesn't auto-align the section but instead just errors out when the section isn't aligned. So you would write
RW_VPD(PRESERVE,ALIGNED) 32K
and then if at any point you change something else in the file such that this ends up at a non-aligned boundary, you'll get a build-time error. The assumption there is that the only alignment we'd care about is 4K for the erase block.
That is probably the better approach.
-
Arthur Heymans (Code Review) -
build bot (Jenkins) (Code Review)
-
Julius Werner (Code Review)