Patch set updated for coreboot: c5d61e5 cbfstool: Extend "locate" command to support alignment and page-fitting.
Hung-Te Lin (hungte@chromium.org) just uploaded a new patch set to gerrit, which you can find at http://review.coreboot.org/2730
-gerrit
commit c5d61e5a06138f36a486d053e24b6fba1f9b87b5 Author: Hung-Te Lin hungte@chromium.org Date: Fri Mar 15 14:44:05 2013 +0800
cbfstool: Extend "locate" command to support alignment and page-fitting.
cbfstool usage change: The "-a" parameter for "cbfstool locate" has changed to "-P/--page-size". "-a" for "cbfstool locate" is now for specifying base address alignment.
The "locate" command was used to find a place to store ELF stage image in one memory page. Its argument "-a (alignment)" was actually specifying the page size instead of doing memory address alignment. This can be confusing when people are trying to put a blob in aligned location (ex, microcode needs to be aligned in 0x10), and see this: cbfstool coreboot.rom localte -f test.bin -n test -a 0x100 # output: 0x44, which does not look like aligned to 0x100.
To support that and prevent confusion, we should rename the page-fitting argument to "-P/--page-size" and leave "-a/--alignment" for real alignment.
Verified by manually testing a file (324 bytes) with alignment=0x10: cbfstool coreboot.rom locate -f test -n test -a 0x10 # output: 0x71fdd0 cbfstool coreboot.rom add -f test -n test -t raw -b 0x71fdd0 cbfstool coreboot.rom print -v -v # output: test 0x71fd80 raw 324 # output: cbfs_file=0x71fd80, offset=0x50, content_address=0x71fdd0+0x144
And verified to be compatible with old behavior by building i386/axus/tc320 (with page limitation 0x40000): cbfstool coreboot.rom locate -f romstage_null.bin -n romstage -P 0x40000 # output: 0x44 cbfstool coreboot.rom locate -f x.bin -n romstage -P 0x40000 -a 0x30 # output: 0x60
Change-Id: I0893adde51ebf46da1c34913f9c35507ed8ff731 Signed-off-by: Hung-Te Lin hungte@chromium.org --- src/arch/armv7/Makefile.inc | 2 +- src/arch/x86/Makefile.inc | 2 +- util/cbfstool/cbfs_image.c | 71 +++++++++++++++++++++++++++++++-------------- util/cbfstool/cbfs_image.h | 6 ++-- util/cbfstool/cbfstool.c | 15 ++++++---- 5 files changed, 65 insertions(+), 31 deletions(-)
diff --git a/src/arch/armv7/Makefile.inc b/src/arch/armv7/Makefile.inc index 39c115e..8601112 100644 --- a/src/arch/armv7/Makefile.inc +++ b/src/arch/armv7/Makefile.inc @@ -282,7 +282,7 @@ $(objgenerated)/romstage_xip.ld: $(objgenerated)/romstage_null.ld $(objcbfs)/bas $(objcbfs)/base_xip.txt: $(obj)/coreboot.pre1 $(objcbfs)/romstage_null.bin @printf " generating base_xip.txt\n" rm -f $@ - $(CBFSTOOL) $(obj)/coreboot.pre1 locate -f $(objcbfs)/romstage_null.bin -n $(CONFIG_CBFS_PREFIX)/romstage -a $(CONFIG_XIP_ROM_SIZE) > $@.tmp \ + $(CBFSTOOL) $(obj)/coreboot.pre1 locate -f $(objcbfs)/romstage_null.bin -n $(CONFIG_CBFS_PREFIX)/romstage -P $(CONFIG_XIP_ROM_SIZE) > $@.tmp \ || { echo "The romstage is larger than XIP size. Please expand the CONFIG_XIP_ROM_SIZE" ; exit 1; } mv $@.tmp $@
diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc index 0892efd..3c57701 100644 --- a/src/arch/x86/Makefile.inc +++ b/src/arch/x86/Makefile.inc @@ -387,7 +387,7 @@ $(objgenerated)/romstage_xip.ld: $(objgenerated)/romstage_null.ld $(objcbfs)/bas
$(objcbfs)/base_xip.txt: $(obj)/coreboot.pre1 $(objcbfs)/romstage_null.bin rm -f $@ - $(CBFSTOOL) $(obj)/coreboot.pre1 locate -T -f $(objcbfs)/romstage_null.bin -n $(CONFIG_CBFS_PREFIX)/romstage -a $(CONFIG_XIP_ROM_SIZE) > $@.tmp \ + $(CBFSTOOL) $(obj)/coreboot.pre1 locate -T -f $(objcbfs)/romstage_null.bin -n $(CONFIG_CBFS_PREFIX)/romstage -P $(CONFIG_XIP_ROM_SIZE) > $@.tmp \ || { echo "The romstage is larger than XIP size. Please expand the CONFIG_XIP_ROM_SIZE" ; exit 1; } mv $@.tmp $@
diff --git a/util/cbfstool/cbfs_image.c b/util/cbfstool/cbfs_image.c index 2cdbf23..63d6f78 100644 --- a/util/cbfstool/cbfs_image.c +++ b/util/cbfstool/cbfs_image.c @@ -773,7 +773,7 @@ int cbfs_create_empty_entry(struct cbfs_image *image, struct cbfs_file *entry, return 0; }
-/* Finds a place to hold whole stage data in same memory page. +/* Finds a place to hold whole data in same memory page. */ static int is_in_same_page(uint32_t start, uint32_t size, uint32_t page) { if (!page) @@ -781,24 +781,44 @@ static int is_in_same_page(uint32_t start, uint32_t size, uint32_t page) { return (start / page) == (start + size - 1) / page; }
+/* Tests if data can fit in a range by given offset: + * start ->| header_len | offset (+ size) |<- end + */ +static int is_in_range(uint32_t start, uint32_t end, uint32_t header_len, + uint32_t offset, uint32_t size) { + return (offset >= start + header_len && offset + size <= end); +} + int32_t cbfs_locate_entry(struct cbfs_image *image, const char *name, - uint32_t size, uint32_t page_size) { + uint32_t size, uint32_t page_size, uint32_t align) { struct cbfs_file *entry; size_t need_len; - uint32_t addr, addr_next, addr2, addr3, header_len; - assert(size < page_size); + uint32_t addr, addr_next, addr2, addr3, offset, header_len; + + /* Default values: allow fitting anywhere in ROM. */ + if (!page_size) + page_size = ntohl(image->header->romsize); + if (!align) + align = 1; + + if (size > page_size) + ERROR("Input file size (%d) greater than page size (%d).\n", + size, page_size);
if (page_size % ntohl(image->header->align)) - WARN("locate_entry: page does not align with CBFS image.\n"); + WARN("%s: Page size (%#x) not aligned with CBFS image (%#x).\n", + __func__, page_size, ntohl(image->header->align));
/* TODO Old cbfstool always assume input is a stage file (and adding * sizeof(cbfs_stage) for header. We should fix that by adding "-t" - * (type) param in future. For right now, follow old behavior. */ + * (type) param in future. For right now, we assume cbfs_stage is the + * largest structure and add it into header size. */ + assert(sizeof(struct cbfs_stage) >= sizeof(struct cbfs_payload)); header_len = (cbfs_calculate_file_header_size(name) + sizeof(struct cbfs_stage)); need_len = header_len + size;
- // Merge empty entries to build get max available pages. + // Merge empty entries to build get max available space. cbfs_walk(image, cbfs_merge_empty_entry, NULL);
/* Three cases of content location on memory page: @@ -812,14 +832,15 @@ int32_t cbfs_locate_entry(struct cbfs_image *image, const char *name, * shift-> | <header>|<content> | at starting of PAGE 2. * * case 3. (large content filling whole page) - * | PAGE 1 | PAGE 2 | PAGE 3| - * | <header>< content > | | Can't fit. If we shift content to - * | { free space . } PAGE 2, header can't fit in free - * | shift-> <header><content> space, so we must use PAGE 3. + * | PAGE 1 | PAGE 2 | PAGE 3 | + * | <header>< content > | Can't fit. If we shift content to + * |trial-> <header>< content > | PAGE 2, header can't fit in free + * | shift-> <header><content> space, so we must use PAGE 3. * - * The returned address will be used to re-link stage file, and then - * assigned to add-stage command (-b), which will be then re-calculated - * by ELF loader and positioned by cbfs_add_entry. + * The returned address can be then used as "base-address" (-b) in add-* + * commands (will be re-calculated and positioned by cbfs_add_entry_at). + * For stage targets, the address is also used to re-link stage before + * being added into CBFS. */ for (entry = cbfs_find_first_entry(image); entry && cbfs_is_valid_entry(image, entry); @@ -834,23 +855,29 @@ int32_t cbfs_locate_entry(struct cbfs_image *image, const char *name, image, entry)); if (addr_next - addr < need_len) continue; - if (is_in_same_page(addr + header_len, size, page_size)) { + + offset = align_up(addr + header_len, align); + if (is_in_same_page(offset, size, page_size) && + is_in_range(addr, addr_next, header_len, offset, size)) { DEBUG("cbfs_locate_entry: FIT (PAGE1)."); - return addr + header_len; + return offset; }
addr2 = align_up(addr, page_size); - if (addr2 < addr_next && addr_next - addr2 >= size && - addr2 - addr >= header_len) { + offset = align_up(addr2, align); + if (is_in_range(addr, addr_next, header_len, offset, size)) { DEBUG("cbfs_locate_entry: OVERLAP (PAGE2)."); - return addr2; + return offset; }
+ /* Assume page_size >= header_len so adding one page will + * definitely provide the space for header. */ + assert(page_size >= header_len); addr3 = addr2 + page_size; - if (addr3 < addr_next && addr_next - addr3 >= size && - addr3 - addr >= header_len) { + offset = align_up(addr3, align); + if (is_in_range(addr, addr_next, header_len, offset, size)) { DEBUG("cbfs_locate_entry: OVERLAP+ (PAGE3)."); - return addr3; + return offset; } } return -1; diff --git a/util/cbfstool/cbfs_image.h b/util/cbfstool/cbfs_image.h index 57bbfa1..7ad418a 100644 --- a/util/cbfstool/cbfs_image.h +++ b/util/cbfstool/cbfs_image.h @@ -76,10 +76,12 @@ int cbfs_remove_entry(struct cbfs_image *image, const char *name); int cbfs_create_empty_entry(struct cbfs_image *image, struct cbfs_file *entry, size_t len, const char *name);
-/* Finds a location to put given content in same memory page. +/* Finds a location to put given content by specified criteria: + * "page_size" limits the content to fit on same memory page, and + * "align" specifies starting address alignment. * Returns a valid offset, or -1 on failure. */ int32_t cbfs_locate_entry(struct cbfs_image *image, const char *name, - uint32_t size, uint32_t page_size); + uint32_t size, uint32_t page_size, uint32_t align);
/* Callback function used by cbfs_walk. * Returns 0 on success, or non-zero to stop further iteration. */ diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index 97fd88d..7eca01d 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -50,6 +50,7 @@ static struct param { uint32_t entrypoint; uint32_t size; uint32_t alignment; + uint32_t pagesize; uint32_t offset; uint32_t top_aligned; comp_algo algo; @@ -355,12 +356,12 @@ static int cbfs_locate(void) }
address = cbfs_locate_entry(&image, param.name, buffer.size, - param.alignment); + param.pagesize, param.alignment); buffer_delete(&buffer);
if (address == -1) { - ERROR("'%s' can't fit in CBFS for align 0x%x.\n", - param.name, param.alignment); + ERROR("'%s' can't fit in CBFS for page-size %#x, align %#x.\n", + param.name, param.pagesize, param.alignment); cbfs_image_delete(&image); return 1; } @@ -421,7 +422,7 @@ static const struct command commands[] = { {"add-flat-binary", "f:n:l:e:c:b:vh?", cbfs_add_flat_binary}, {"remove", "n:vh?", cbfs_remove}, {"create", "s:B:b:H:a:o:m:vh?", cbfs_create}, - {"locate", "f:n:a:Tvh?", cbfs_locate}, + {"locate", "f:n:a:P:Tvh?", cbfs_locate}, {"print", "vh?", cbfs_print}, {"extract", "n:f:vh?", cbfs_extract}, }; @@ -437,6 +438,7 @@ static struct option long_options[] = { {"size", required_argument, 0, 's' }, {"bootblock", required_argument, 0, 'B' }, {"alignment", required_argument, 0, 'a' }, + {"page-size", required_argument, 0, 'P' }, {"offset", required_argument, 0, 'o' }, {"file", required_argument, 0, 'f' }, {"arch", required_argument, 0, 'm' }, @@ -468,7 +470,7 @@ static void usage(char *name) "Remove a component\n" " create -s size -B bootblock -m ARCH [-a align] [-o offset] " "Create a ROM file\n" - " locate -f FILE -n NAME [-a align] [-T] " + " locate -f FILE -n NAME [-P page-size] [-a align] [-T] " "Find a place for a file of that size\n" " print " "Show the contents of the ROM\n" @@ -572,6 +574,9 @@ int main(int argc, char **argv) case 'a': param.alignment = strtoul(optarg, NULL, 0); break; + case 'P': + param.pagesize = strtoul(optarg, NULL, 0); + break; case 'o': param.offset = strtoul(optarg, NULL, 0); break;
-
Hung-Te Lin