[coreboot] Patch set updated for coreboot: 0273393 cbfstool: Extend "locate" command to support alignment and page-fitting.

Hung-Te Lin (hungte@chromium.org) gerrit at coreboot.org
Mon Mar 18 05:43:51 CET 2013


Hung-Te Lin (hungte at chromium.org) just uploaded a new patch set to gerrit, which you can find at http://review.coreboot.org/2730

-gerrit

commit 0273393fccaf7922473f8404f2a45bd2fe19e220
Author: Hung-Te Lin <hungte at 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 at 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 cc7bfc2..433b434 100644
--- a/src/arch/x86/Makefile.inc
+++ b/src/arch/x86/Makefile.inc
@@ -393,7 +393,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;



More information about the coreboot mailing list