Hello Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/47827
to review the following change.
Change subject: cbfstool: Replace FILENAME_ALIGN 16 with ATTRIBUTE_ALIGN 4 ......................................................................
cbfstool: Replace FILENAME_ALIGN 16 with ATTRIBUTE_ALIGN 4
cbfstool has always had a CBFS_FILENAME_ALIGN that forces the filename field to be aligned upwards to the next 16-byte boundary. This was presumably done to align the file contents (which used to come immediately after the filename field).
However, this hasn't really worked right ever since we introduced CBFS attributes. Attributes come between the filename and the contents, so what this code currently does is fill up the filename field with extra NUL-bytes to the boundary, and then just put the attributes behind it with whatever size they may be. The file contents don't end up with any alignment guarantee and the filename field is just wasting space.
This patch removes the old FILENAME_ALIGN, and instead adds a new alignment of 4 for the attributes. 4 seems like a reasonable alignment to enforce since all existing attributes (with the exception of weird edge cases with the padding attribute) already use sizes divisible by 4 anyway, and the common attribute header fields have a natural alignment of 4. This means file contents will also have a minimum alignment guarantee of 4 -- files requiring a larger guarantee can still be added with the --alignment flag as usual.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I43f3906977094df87fdc283221d8971a6df01b53 --- M src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h M util/cbfstool/cbfs_image.c M util/cbfstool/cbfstool.c 3 files changed, 7 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/47827/1
diff --git a/src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h b/src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h index c31d995..7cdea26 100644 --- a/src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h +++ b/src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h @@ -115,6 +115,9 @@ uint8_t data[0]; } __packed;
+/* All attribute sizes must be divisible by this! */ +#define CBFS_ATTRIBUTE_ALIGN 4 + /* Depending on how the header was initialized, it may be backed with 0x00 or * 0xff. Support both. */ enum cbfs_file_attr_tag { diff --git a/util/cbfstool/cbfs_image.c b/util/cbfstool/cbfs_image.c index c7531d6..20df20b 100644 --- a/util/cbfstool/cbfs_image.c +++ b/util/cbfstool/cbfs_image.c @@ -34,10 +34,6 @@ * removing said guarantees. */
-/* The file name align is not defined in CBFS spec -- only a preference by - * (old) cbfstool. */ -#define CBFS_FILENAME_ALIGN (16) - static const char *lookup_name_by_type(const struct typedesc_t *desc, uint32_t type, const char *default_value) { @@ -70,7 +66,7 @@ size_t cbfs_calculate_file_header_size(const char *name) { return (sizeof(struct cbfs_file) + - align_up(strlen(name) + 1, CBFS_FILENAME_ALIGN)); + align_up(strlen(name) + 1, CBFS_ATTRIBUTE_ALIGN)); }
/* Only call on legacy CBFSes possessing a master header. */ @@ -1836,6 +1832,7 @@ uint32_t tag, uint32_t size) { + assert(IS_ALIGNED(size, CBFS_ATTRIBUTE_ALIGN)); struct cbfs_file_attribute *attr, *next; next = cbfs_file_first_attr(header); do { diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index 68c2c22..8df0fa1 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -740,7 +740,8 @@
if (param.padding) { const uint32_t hs = sizeof(struct cbfs_file_attribute); - uint32_t size = MAX(hs, param.padding); + uint32_t size = ALIGN_UP(MAX(hs, param.padding), + CBFS_ATTRIBUTE_ALIGN); INFO("Padding %d bytes\n", size); struct cbfs_file_attribute *attr = (struct cbfs_file_attribute *)cbfs_add_file_attr(
Hello Patrick Georgi, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47827
to look at the new patch set (#2).
Change subject: cbfstool: Replace FILENAME_ALIGN 16 with ATTRIBUTE_ALIGN 4 ......................................................................
cbfstool: Replace FILENAME_ALIGN 16 with ATTRIBUTE_ALIGN 4
cbfstool has always had a CBFS_FILENAME_ALIGN that forces the filename field to be aligned upwards to the next 16-byte boundary. This was presumably done to align the file contents (which used to come immediately after the filename field).
However, this hasn't really worked right ever since we introduced CBFS attributes. Attributes come between the filename and the contents, so what this code currently does is fill up the filename field with extra NUL-bytes to the boundary, and then just put the attributes behind it with whatever size they may be. The file contents don't end up with any alignment guarantee and the filename field is just wasting space.
This patch removes the old FILENAME_ALIGN, and instead adds a new alignment of 4 for the attributes. 4 seems like a reasonable alignment to enforce since all existing attributes (with the exception of weird edge cases with the padding attribute) already use sizes divisible by 4 anyway, and the common attribute header fields have a natural alignment of 4. This means file contents will also have a minimum alignment guarantee of 4 -- files requiring a larger guarantee can still be added with the --alignment flag as usual.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I43f3906977094df87fdc283221d8971a6df01b53 --- M src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h M util/cbfstool/cbfs_image.c M util/cbfstool/cbfstool.c 3 files changed, 7 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/47827/2
Attention is currently required from: Furquan Shaikh. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47827 )
Change subject: cbfstool: Replace FILENAME_ALIGN 16 with ATTRIBUTE_ALIGN 4 ......................................................................
Patch Set 9:
(1 comment)
Patchset:
PS9: I must be dumb... why does this say FAILURE but when I click the link it just says "no test failed"? Where's the error?
Attention is currently required from: Furquan Shaikh. Julius Werner has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/47827 )
Change subject: cbfstool: Replace FILENAME_ALIGN 16 with ATTRIBUTE_ALIGN 4 ......................................................................
Removed Verified-1 by build bot (Jenkins) no-reply@coreboot.org
Attention is currently required from: Furquan Shaikh. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47827 )
Change subject: cbfstool: Replace FILENAME_ALIGN 16 with ATTRIBUTE_ALIGN 4 ......................................................................
Patch Set 9:
(1 comment)
Patchset:
PS9:
I must be dumb... […]
Was a builder issue.
Attention is currently required from: Furquan Shaikh, Julius Werner. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47827 )
Change subject: cbfstool: Replace FILENAME_ALIGN 16 with ATTRIBUTE_ALIGN 4 ......................................................................
Patch Set 11: Code-Review+1
(1 comment)
File src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h:
https://review.coreboot.org/c/coreboot/+/47827/comment/4b546f14_a311d2c6 PS11, Line 118: #define CBFS_ATTRIBUTE_ALIGN 4 Should we add an _Static_assert for existing attribute structs to guarantee that? This would also make the alignment requirement easier to notice if someone were to add a new attribute in the future.
I mean, if *I* were to add a new attribute and saw there's an _Static_assert for each existing attribute, I would look at the definition of `CBFS_ATTRIBUTE_ALIGN` and notice the comment. But even if one bluntly copy-pasted an existing entry, one would probably copy-paste the _Static_assert too.
Attention is currently required from: Furquan Shaikh, Julius Werner. Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47827 )
Change subject: cbfstool: Replace FILENAME_ALIGN 16 with ATTRIBUTE_ALIGN 4 ......................................................................
Patch Set 11: Code-Review+2
Attention is currently required from: Furquan Shaikh, Angel Pons. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47827 )
Change subject: cbfstool: Replace FILENAME_ALIGN 16 with ATTRIBUTE_ALIGN 4 ......................................................................
Patch Set 11:
(1 comment)
File src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h:
https://review.coreboot.org/c/coreboot/+/47827/comment/99205c0d_f46b183a PS11, Line 118: #define CBFS_ATTRIBUTE_ALIGN 4
Should we add an _Static_assert for existing attribute structs to guarantee that? This would also ma […]
I mean, I could do it for the structs that are fixed-size, but the problem is that not all of them are. So my solution here is basically just to put that (non-static) assert into cbfstool, and then if you added an attribute like that you'd still notice very quickly when trying out your code.
Attention is currently required from: Furquan Shaikh, Julius Werner. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47827 )
Change subject: cbfstool: Replace FILENAME_ALIGN 16 with ATTRIBUTE_ALIGN 4 ......................................................................
Patch Set 12:
(1 comment)
File src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h:
https://review.coreboot.org/c/coreboot/+/47827/comment/4dc19e46_a0f8e1ad PS11, Line 118: #define CBFS_ATTRIBUTE_ALIGN 4
I mean, I could do it for the structs that are fixed-size, but the problem is that not all of them a […]
Ack
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47827 )
Change subject: cbfstool: Replace FILENAME_ALIGN 16 with ATTRIBUTE_ALIGN 4 ......................................................................
cbfstool: Replace FILENAME_ALIGN 16 with ATTRIBUTE_ALIGN 4
cbfstool has always had a CBFS_FILENAME_ALIGN that forces the filename field to be aligned upwards to the next 16-byte boundary. This was presumably done to align the file contents (which used to come immediately after the filename field).
However, this hasn't really worked right ever since we introduced CBFS attributes. Attributes come between the filename and the contents, so what this code currently does is fill up the filename field with extra NUL-bytes to the boundary, and then just put the attributes behind it with whatever size they may be. The file contents don't end up with any alignment guarantee and the filename field is just wasting space.
This patch removes the old FILENAME_ALIGN, and instead adds a new alignment of 4 for the attributes. 4 seems like a reasonable alignment to enforce since all existing attributes (with the exception of weird edge cases with the padding attribute) already use sizes divisible by 4 anyway, and the common attribute header fields have a natural alignment of 4. This means file contents will also have a minimum alignment guarantee of 4 -- files requiring a larger guarantee can still be added with the --alignment flag as usual.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I43f3906977094df87fdc283221d8971a6df01b53 Reviewed-on: https://review.coreboot.org/c/coreboot/+/47827 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h M util/cbfstool/cbfs_image.c M util/cbfstool/cbfstool.c 3 files changed, 7 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve
diff --git a/src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h b/src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h index ac4b38f..dd50469 100644 --- a/src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h +++ b/src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h @@ -114,6 +114,9 @@ uint8_t data[0]; } __packed;
+/* All attribute sizes must be divisible by this! */ +#define CBFS_ATTRIBUTE_ALIGN 4 + /* Depending on how the header was initialized, it may be backed with 0x00 or * 0xff. Support both. */ enum cbfs_file_attr_tag { diff --git a/util/cbfstool/cbfs_image.c b/util/cbfstool/cbfs_image.c index 0191682..d3c6c94 100644 --- a/util/cbfstool/cbfs_image.c +++ b/util/cbfstool/cbfs_image.c @@ -34,10 +34,6 @@ * removing said guarantees. */
-/* The file name align is not defined in CBFS spec -- only a preference by - * (old) cbfstool. */ -#define CBFS_FILENAME_ALIGN (16) - static const char *lookup_name_by_type(const struct typedesc_t *desc, uint32_t type, const char *default_value) { @@ -70,7 +66,7 @@ size_t cbfs_calculate_file_header_size(const char *name) { return (sizeof(struct cbfs_file) + - align_up(strlen(name) + 1, CBFS_FILENAME_ALIGN)); + align_up(strlen(name) + 1, CBFS_ATTRIBUTE_ALIGN)); }
/* Only call on legacy CBFSes possessing a master header. */ @@ -1870,6 +1866,7 @@ uint32_t tag, uint32_t size) { + assert(IS_ALIGNED(size, CBFS_ATTRIBUTE_ALIGN)); struct cbfs_file_attribute *attr, *next; next = cbfs_file_first_attr(header); do { diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index c7330a4..72c01b9 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -751,7 +751,8 @@
if (param.padding) { const uint32_t hs = sizeof(struct cbfs_file_attribute); - uint32_t size = MAX(hs, param.padding); + uint32_t size = ALIGN_UP(MAX(hs, param.padding), + CBFS_ATTRIBUTE_ALIGN); INFO("Padding %d bytes\n", size); struct cbfs_file_attribute *attr = (struct cbfs_file_attribute *)cbfs_add_file_attr(
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Julius Werner (Code Review)