From 5779ca718c84207f8ba14daaf74aa919d0c36d0a Mon Sep 17 00:00:00 2001 From: Julius Werner Date: Fri, 20 Nov 2020 16:12:40 -0800 Subject: 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 Change-Id: I43f3906977094df87fdc283221d8971a6df01b53 Reviewed-on: https://review.coreboot.org/c/coreboot/+/47827 Tested-by: build bot (Jenkins) Reviewed-by: Angel Pons Reviewed-by: Aaron Durbin --- src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h | 3 +++ util/cbfstool/cbfs_image.c | 7 ++----- util/cbfstool/cbfstool.c | 3 ++- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h b/src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h index ac4b38f7a2..dd504695b3 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 @@ struct cbfs_file_attribute { 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 0191682de9..d3c6c94d48 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 @@ int cbfs_parse_comp_algo(const char *name) 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 @@ struct cbfs_file_attribute *cbfs_add_file_attr(struct cbfs_file *header, 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 c7330a493e..72c01b9ea0 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -751,7 +751,8 @@ static int cbfs_add_component(const char *filename, 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( -- cgit v1.2.3