The current test for the 'entry' module is a bit convoluted since it has to import the module multiple times. It also relies on ordering, in that test1EntryNoImportLib() must run before test2EntryImportLib() if they are running in the same Python process.

This is unreliable since neither the ordering of tests nor the process that they run in is defined.

Fix this by always reloading the entry in these two tests. Also add a check that the expected value of have_importlib is obtained.

This corrects a code-coverage problem in the 'entry' module on some systems.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/entry_test.py | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/tools/binman/entry_test.py b/tools/binman/entry_test.py index b30a7beecc8..b6ad3edb8dc 100644 --- a/tools/binman/entry_test.py +++ b/tools/binman/entry_test.py @@ -9,12 +9,11 @@ import os import sys import unittest

+import entry import fdt import fdt_util import tools

-entry = None - class TestEntry(unittest.TestCase): def setUp(self): tools.PrepareOutputDir(None) @@ -29,16 +28,7 @@ class TestEntry(unittest.TestCase): dtb = fdt.FdtScan(fname) return dtb.GetNode('/binman/u-boot')

- def test1EntryNoImportLib(self): - """Test that we can import Entry subclassess successfully""" - - sys.modules['importlib'] = None - global entry - import entry - entry.Entry.Create(None, self.GetNode(), 'u-boot') - - def test2EntryImportLib(self): - del sys.modules['importlib'] + def _ReloadEntry(self): global entry if entry: if sys.version_info[0] >= 3: @@ -48,8 +38,21 @@ class TestEntry(unittest.TestCase): reload(entry) else: import entry + + def test1EntryNoImportLib(self): + """Test that we can import Entry subclassess successfully""" + sys.modules['importlib'] = None + global entry + self._ReloadEntry() + entry.Entry.Create(None, self.GetNode(), 'u-boot') + self.assertFalse(entry.have_importlib) + + def test2EntryImportLib(self): + del sys.modules['importlib'] + global entry + self._ReloadEntry() entry.Entry.Create(None, self.GetNode(), 'u-boot-spl') - del entry + self.assertTrue(entry.have_importlib)

def testEntryContents(self): """Test the Entry bass class""" @@ -59,7 +62,6 @@ class TestEntry(unittest.TestCase):

def testUnknownEntry(self): """Test that unknown entry types are detected""" - import entry Node = collections.namedtuple('Node', ['name', 'path']) node = Node('invalid-name', 'invalid-path') with self.assertRaises(ValueError) as e: @@ -69,7 +71,6 @@ class TestEntry(unittest.TestCase):

def testUniqueName(self): """Test Entry.GetUniqueName""" - import entry Node = collections.namedtuple('Node', ['name', 'parent']) base_node = Node('root', None) base_entry = entry.Entry(None, None, base_node, read_node=False) @@ -80,7 +81,6 @@ class TestEntry(unittest.TestCase):

def testGetDefaultFilename(self): """Trivial test for this base class function""" - import entry base_entry = entry.Entry(None, None, None, read_node=False) self.assertIsNone(base_entry.GetDefaultFilename())

A few features have been completed and a few items are added.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/README | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/tools/binman/README b/tools/binman/README index 2f4c7fec21e..1851a4e536d 100644 --- a/tools/binman/README +++ b/tools/binman/README @@ -811,13 +811,17 @@ Some ideas: - Use of-platdata to make the information available to code that is unable to use device tree (such as a very small SPL image) - Allow easy building of images by specifying just the board name -- Produce a full Python binding for libfdt (for upstream). This is nearing - completion but some work remains - Add an option to decode an image into the constituent binaries - Support building an image for a board (-b) more completely, with a configurable build directory -- Consider making binman work with buildman, although if it is used in the - Makefile, this will be automatic +- Support putting the FDT in an image with a suitable magic number +- Support adding a pointer to the FDT map +- Support listing files in images +- Support logging of binman's operations, with different levels of verbosity +- Support updating binaries in an image (with no size change / repacking) +- Support updating binaries in an image (with repacking) +- Support adding FITs to an image +- Support for ARM Trusted Firmware (ATF)

-- Simon Glass sjg@chromium.org

A few new features have been added. This has rendered part of the README obsolete. Update it.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/README | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/binman/README b/tools/binman/README index 1851a4e536d..b067d3b78cf 100644 --- a/tools/binman/README +++ b/tools/binman/README @@ -36,10 +36,9 @@ suitable padding and alignment. It provides a way to process binaries before they are included, by adding a Python plug-in. The device tree is available to U-Boot at run-time so that the images can be interpreted.

-Binman does not yet update the device tree with the final location of -everything when it is done. A simple C structure could be generated for -constrained environments like SPL (using dtoc) but this is also not -implemented. +Binman can update the device tree with the final location of everything when it +is done. Entry positions can be provided to U-Boot SPL as run-time symbols, +avoiding device-tree code overhead.

Binman can also support incorporating filesystems in the image if required. For example x86 platforms may use CBFS in some cases.

Quite a few tests will use a real device tree and need it updated with the binman metadata. Add a helper function for this.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/ftest.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 5bde8aa30a1..ed125670e2c 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -380,6 +380,17 @@ class TestFunctional(unittest.TestCase): if reset_dtbs and use_real_dtb: self._ResetDtbs()

+ def _DoReadFileRealDtb(self, fname): + """Run binman with a real .dtb file and return the resulting data + + Args: + fname: DT source filename to use (e.g. 082_fdt_update_all.dts) + + Returns: + Resulting image contents + """ + return self._DoReadFileDtb(fname, use_real_dtb=True, update_dtb=True)[0] + def _DoReadFile(self, fname, use_real_dtb=False): """Helper function which discards the device-tree binary

@@ -1547,8 +1558,7 @@ class TestFunctional(unittest.TestCase):

def testUpdateFdtAll(self): """Test that all device trees are updated with offset/size info""" - data, _, _, _ = self._DoReadFileDtb('082_fdt_update_all.dts', - use_real_dtb=True, update_dtb=True) + data = self._DoReadFileRealDtb('082_fdt_update_all.dts')

base_expected = { 'section:image-pos': 0,

An FDT map is an entry which holds a full description of the image entries, in FDT format. It can be discovered using the magic string at its start. Tools can locate and read this entry to find out what entries are in the image and where each entry is located.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/README | 5 +- tools/binman/README.entries | 38 +++++++++++ tools/binman/etype/fdtmap.py | 109 +++++++++++++++++++++++++++++++ tools/binman/ftest.py | 52 +++++++++++++-- tools/binman/state.py | 2 +- tools/binman/test/115_fdtmap.dts | 13 ++++ 6 files changed, 211 insertions(+), 8 deletions(-) create mode 100644 tools/binman/etype/fdtmap.py create mode 100644 tools/binman/test/115_fdtmap.dts

diff --git a/tools/binman/README b/tools/binman/README index b067d3b78cf..8a5f3320dcb 100644 --- a/tools/binman/README +++ b/tools/binman/README @@ -638,6 +638,10 @@ the image definition, binman calculates the final values and writes these to the device tree. These can be used by U-Boot at run-time to find the location of each entry.

+Alternatively, an FDT map entry can be used to add a special FDT containing +just the information about the image. This is preceded by a magic string so can +be located anywhere in the image. +

Compression ----------- @@ -814,7 +818,6 @@ Some ideas: - Support building an image for a board (-b) more completely, with a configurable build directory - Support putting the FDT in an image with a suitable magic number -- Support adding a pointer to the FDT map - Support listing files in images - Support logging of binman's operations, with different levels of verbosity - Support updating binaries in an image (with no size change / repacking) diff --git a/tools/binman/README.entries b/tools/binman/README.entries index 3241befc7f4..7014d36f5ff 100644 --- a/tools/binman/README.entries +++ b/tools/binman/README.entries @@ -223,6 +223,44 @@ updating the EC on startup via software sync.

+Entry: fdtmap: An entry which contains an FDT map +------------------------------------------------- + +Properties / Entry arguments: + None + +An FDT map is just a header followed by an FDT containing a list of all the +entries in the image. + +The header is the string _FDTMAP_ followed by 8 unused bytes. + +When used, this entry will be populated with an FDT map which reflects the +entries in the current image. Hierarchy is preserved, and all offsets and +sizes are included. + +Note that the -u option must be provided to ensure that binman updates the +FDT with the position of each entry. + +Example output for a simple image with U-Boot and an FDT map: + +/ { + size = <0x00000112>; + image-pos = <0x00000000>; + offset = <0x00000000>; + u-boot { + size = <0x00000004>; + image-pos = <0x00000000>; + offset = <0x00000000>; + }; + fdtmap { + size = <0x0000010e>; + image-pos = <0x00000004>; + offset = <0x00000004>; + }; +}; + + + Entry: files: Entry containing a set of files ---------------------------------------------

diff --git a/tools/binman/etype/fdtmap.py b/tools/binman/etype/fdtmap.py new file mode 100644 index 00000000000..cdeb491ebdc --- /dev/null +++ b/tools/binman/etype/fdtmap.py @@ -0,0 +1,109 @@ +# SPDX-License-Identifier: GPL-2.0+ +# Copyright (c) 2018 Google, Inc +# Written by Simon Glass sjg@chromium.org + +"""# Entry-type module for a full map of the firmware image + +This handles putting an FDT into the image with just the information about the +image. +""" + +import libfdt + +from entry import Entry +from fdt import Fdt +import state +import tools + +FDTMAP_MAGIC = b'_FDTMAP_' + +class Entry_fdtmap(Entry): + """An entry which contains an FDT map + + Properties / Entry arguments: + None + + An FDT map is just a header followed by an FDT containing a list of all the + entries in the image. + + The header is the string _FDTMAP_ followed by 8 unused bytes. + + When used, this entry will be populated with an FDT map which reflects the + entries in the current image. Hierarchy is preserved, and all offsets and + sizes are included. + + Note that the -u option must be provided to ensure that binman updates the + FDT with the position of each entry. + + Example output for a simple image with U-Boot and an FDT map: + + / { + size = <0x00000112>; + image-pos = <0x00000000>; + offset = <0x00000000>; + u-boot { + size = <0x00000004>; + image-pos = <0x00000000>; + offset = <0x00000000>; + }; + fdtmap { + size = <0x0000010e>; + image-pos = <0x00000004>; + offset = <0x00000004>; + }; + }; + """ + def __init__(self, section, etype, node): + Entry.__init__(self, section, etype, node) + + def _GetFdtmap(self): + """Build an FDT map from the entries in the current image + + Returns: + FDT map binary data + """ + def _AddNode(node): + """Add a node to the FDT map""" + for pname, prop in node.props.items(): + fsw.property(pname, prop.bytes) + for subnode in node.subnodes: + with fsw.add_node(subnode.name): + _AddNode(subnode) + + # Get the FDT data into an Fdt object + data = state.GetFdtContents()[1] + infdt = Fdt.FromData(data) + infdt.Scan() + + # Find the node for the image containing the Fdt-map entry + path = self.section.GetPath() + node = infdt.GetNode(path) + if not node: + self.Raise("Internal error: Cannot locate node for path '%s'" % + path) + + # Build a new tree with all nodes and properties starting from that node + fsw = libfdt.FdtSw() + fsw.finish_reservemap() + with fsw.add_node(''): + _AddNode(node) + fdt = fsw.as_fdt() + + # Pack this new FDT and return its contents + fdt.pack() + outfdt = Fdt.FromData(fdt.as_bytearray()) + data = FDTMAP_MAGIC + tools.GetBytes(0, 8) + outfdt.GetContents() + return data + + def ObtainContents(self): + """Obtain a placeholder for the fdt-map contents""" + self.SetContents(self._GetFdtmap()) + return True + + def ProcessContents(self): + """Write an updated version of the FDT map to this entry + + This is necessary since new data may have been written back to it during + processing, e.g. the image-pos properties. + """ + self.SetContents(self._GetFdtmap()) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index ed125670e2c..934145ca3cb 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -487,16 +487,16 @@ class TestFunctional(unittest.TestCase): """ return struct.unpack('>L', dtb[4:8])[0]

- def _GetPropTree(self, dtb, prop_names): + def _GetPropTree(self, dtb, prop_names, prefix='/binman/'): def AddNode(node, path): if node.name != '/': path += '/' + node.name + for prop in node.props.values(): + if prop.name in prop_names: + prop_path = path + ':' + prop.name + tree[prop_path[len(prefix):]] = fdt_util.fdt32_to_cpu( + prop.value) for subnode in node.subnodes: - for prop in subnode.props.values(): - if prop.name in prop_names: - prop_path = path + '/' + subnode.name + ':' + prop.name - tree[prop_path[len('/binman/'):]] = fdt_util.fdt32_to_cpu( - prop.value) AddNode(subnode, path)

tree = {} @@ -2044,6 +2044,46 @@ class TestFunctional(unittest.TestCase): self.assertEqual(U_BOOT_DTB_DATA, cfile2.data) self.assertEqual(0x140, cfile2.cbfs_offset)

+ def testFdtmap(self): + """Test an FDT map can be inserted in the image""" + data = self.data = self._DoReadFileRealDtb('115_fdtmap.dts') + fdtmap_data = data[len(U_BOOT_DATA):] + magic = fdtmap_data[:8] + self.assertEqual('_FDTMAP_', magic) + self.assertEqual(tools.GetBytes(0, 8), fdtmap_data[8:16]) + + fdt_data = fdtmap_data[16:] + dtb = fdt.Fdt.FromData(fdt_data) + dtb.Scan() + props = self._GetPropTree(dtb, ['offset', 'size', 'image-pos'], + prefix='/') + self.assertEqual({ + 'image-pos': 0, + 'offset': 0, + 'u-boot:offset': 0, + 'u-boot:size': len(U_BOOT_DATA), + 'u-boot:image-pos': 0, + 'fdtmap:image-pos': 4, + 'fdtmap:offset': 4, + 'fdtmap:size': len(fdtmap_data), + 'size': len(data), + }, props) + + def testFdtmapNoMatch(self): + """Check handling of an FDT map when the section cannot be found""" + self.data = self._DoReadFileRealDtb('115_fdtmap.dts') + + # Mangle the section name, which should cause a mismatch between the + # correct FDT path and the one expected by the section + image = control.images['image'] + image._section._node.path += '-suffix' + entries = image.GetEntries() + fdtmap = entries['fdtmap'] + with self.assertRaises(ValueError) as e: + fdtmap._GetFdtmap() + self.assertIn("Cannot locate node for path '/binman-suffix'", + str(e.exception)) +

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/state.py b/tools/binman/state.py index af9678649cd..3ccd7855d47 100644 --- a/tools/binman/state.py +++ b/tools/binman/state.py @@ -59,7 +59,7 @@ def GetFdtPath(fname): """ return fdt_files[fname]._fname

-def GetFdtContents(fname): +def GetFdtContents(fname='u-boot.dtb'): """Looks up the FDT pathname and contents

This is used to obtain the Fdt pathname and contents when needed by an diff --git a/tools/binman/test/115_fdtmap.dts b/tools/binman/test/115_fdtmap.dts new file mode 100644 index 00000000000..2450c41f200 --- /dev/null +++ b/tools/binman/test/115_fdtmap.dts @@ -0,0 +1,13 @@ +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + u-boot { + }; + fdtmap { + }; + }; +};

It is useful to be able to quickly locate the FDT map in the image. An easy way to do this is with a pointer at the start or end of the image.

Add an 'image header' entry, which places a magic number followed by a pointer to the FDT map. This can be located at the start or end of the image, or at a chosen location.

As part of this, update GetSiblingImagePos() to detect missing siblings.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: - Update test to use _DoReadFileRealDtb() helper

tools/binman/README | 5 +- tools/binman/README.entries | 19 +++++ tools/binman/entry.py | 11 +++ tools/binman/etype/image_header.py | 76 ++++++++++++++++++++ tools/binman/ftest.py | 47 +++++++++++- tools/binman/test/116_fdtmap_hdr.dts | 17 +++++ tools/binman/test/117_fdtmap_hdr_start.dts | 19 +++++ tools/binman/test/118_fdtmap_hdr_pos.dts | 19 +++++ tools/binman/test/119_fdtmap_hdr_missing.dts | 16 +++++ tools/binman/test/120_hdr_no_location.dts | 16 +++++ 10 files changed, 242 insertions(+), 3 deletions(-) create mode 100644 tools/binman/etype/image_header.py create mode 100644 tools/binman/test/116_fdtmap_hdr.dts create mode 100644 tools/binman/test/117_fdtmap_hdr_start.dts create mode 100644 tools/binman/test/118_fdtmap_hdr_pos.dts create mode 100644 tools/binman/test/119_fdtmap_hdr_missing.dts create mode 100644 tools/binman/test/120_hdr_no_location.dts

diff --git a/tools/binman/README b/tools/binman/README index 8a5f3320dcb..ef62d1f1ec7 100644 --- a/tools/binman/README +++ b/tools/binman/README @@ -640,7 +640,9 @@ of each entry.

Alternatively, an FDT map entry can be used to add a special FDT containing just the information about the image. This is preceded by a magic string so can -be located anywhere in the image. +be located anywhere in the image. An image header (typically at the start or end +of the image) can be used to point to the FDT map. See fdtmap and image-header +entries for more information.

Compression @@ -817,7 +819,6 @@ Some ideas: - Add an option to decode an image into the constituent binaries - Support building an image for a board (-b) more completely, with a configurable build directory -- Support putting the FDT in an image with a suitable magic number - Support listing files in images - Support logging of binman's operations, with different levels of verbosity - Support updating binaries in an image (with no size change / repacking) diff --git a/tools/binman/README.entries b/tools/binman/README.entries index 7014d36f5ff..598d8278a70 100644 --- a/tools/binman/README.entries +++ b/tools/binman/README.entries @@ -331,6 +331,25 @@ README.chromium for how to obtain the required keys and tools.

+Entry: image-header: An entry which contains a pointer to the FDT map +--------------------------------------------------------------------- + +Properties / Entry arguments: + location: Location of header ("start" or "end" of image). This is + optional. If omitted then the entry must have an offset property. + +This adds an 8-byte entry to the start or end of the image, pointing to the +location of the FDT map. The format is a magic number followed by an offset +from the start or end of the image, in twos-compliment format. + +This entry must be in the top-level part of the image. + +NOTE: If the location is at the start/end, you will probably need to specify +sort-by-offset for the image, unless you actually put the image header +first/last in the entry list. + + + Entry: intel-cmc: Entry containing an Intel Chipset Micro Code (CMC) file -------------------------------------------------------------------------

diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 7356c49c626..e1cd0d3a882 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -561,3 +561,14 @@ features to produce new behaviours. else False """ return name in self.section.GetEntries() + + def GetSiblingImagePos(self, name): + """Return the image position of the given sibling + + Returns: + Image position of sibling, or None if the sibling has no position, + or False if there is no such sibling + """ + if not self.HasSibling(name): + return False + return self.section.GetEntries()[name].image_pos diff --git a/tools/binman/etype/image_header.py b/tools/binman/etype/image_header.py new file mode 100644 index 00000000000..9bc84ec01d4 --- /dev/null +++ b/tools/binman/etype/image_header.py @@ -0,0 +1,76 @@ +# SPDX-License-Identifier: GPL-2.0+ +# Copyright (c) 2018 Google, Inc +# Written by Simon Glass sjg@chromium.org + +"""Entry-type module for an image header which points to the FDT map + +This creates an 8-byte entry with a magic number and the offset of the FDT map +(which is another entry in the image), relative to the start or end of the +image. +""" + +import struct + +from entry import Entry +import fdt_util + +IMAGE_HEADER_MAGIC = b'BinM' + +class Entry_image_header(Entry): + """An entry which contains a pointer to the FDT map + + Properties / Entry arguments: + location: Location of header ("start" or "end" of image). This is + optional. If omitted then the entry must have an offset property. + + This adds an 8-byte entry to the start or end of the image, pointing to the + location of the FDT map. The format is a magic number followed by an offset + from the start or end of the image, in twos-compliment format. + + This entry must be in the top-level part of the image. + + NOTE: If the location is at the start/end, you will probably need to specify + sort-by-offset for the image, unless you actually put the image header + first/last in the entry list. + """ + def __init__(self, section, etype, node): + Entry.__init__(self, section, etype, node) + self.location = fdt_util.GetString(self._node, 'location') + + def _GetHeader(self): + image_pos = self.GetSiblingImagePos('fdtmap') + if image_pos == False: + self.Raise("'image_header' section must have an 'fdtmap' sibling") + elif image_pos is None: + # This will be available when called from ProcessContents(), but not + # when called from ObtainContents() + offset = 0xffffffff + else: + image_size = self.section.GetImageSize() or 0 + base = (0 if self.location != 'end' else image_size) + offset = (image_pos - base) & 0xffffffff + data = IMAGE_HEADER_MAGIC + struct.pack('<I', offset) + return data + + def ObtainContents(self): + """Obtain a placeholder for the header contents""" + self.SetContents(self._GetHeader()) + return True + + def Pack(self, offset): + """Special pack method to set the offset to start/end of image""" + if not self.offset: + if self.location not in ['start', 'end']: + self.Raise("Invalid location '%s', expected 'start' or 'end'" % + self.location) + image_size = self.section.GetImageSize() or 0 + self.offset = (0 if self.location != 'end' else image_size - 8) + return Entry.Pack(self, offset) + + def ProcessContents(self): + """Write an updated version of the FDT map to this entry + + This is necessary since image_pos is not available when ObtainContents() + is called, since by then the entries have not been packed in the image. + """ + self.SetContents(self._GetHeader()) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 934145ca3cb..6fdecb2f3bf 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -2076,7 +2076,7 @@ class TestFunctional(unittest.TestCase): # Mangle the section name, which should cause a mismatch between the # correct FDT path and the one expected by the section image = control.images['image'] - image._section._node.path += '-suffix' + image._node.path += '-suffix' entries = image.GetEntries() fdtmap = entries['fdtmap'] with self.assertRaises(ValueError) as e: @@ -2084,6 +2084,51 @@ class TestFunctional(unittest.TestCase): self.assertIn("Cannot locate node for path '/binman-suffix'", str(e.exception))

+ def testFdtmapHeader(self): + """Test an FDT map and image header can be inserted in the image""" + data = self.data = self._DoReadFileRealDtb('116_fdtmap_hdr.dts') + fdtmap_pos = len(U_BOOT_DATA) + fdtmap_data = data[fdtmap_pos:] + fdt_data = fdtmap_data[16:] + dtb = fdt.Fdt.FromData(fdt_data) + fdt_size = dtb.GetFdtObj().totalsize() + hdr_data = data[-8:] + self.assertEqual('BinM', hdr_data[:4]) + offset = struct.unpack('<I', hdr_data[4:])[0] & 0xffffffff + self.assertEqual(fdtmap_pos - 0x400, offset - (1 << 32)) + + def testFdtmapHeaderStart(self): + """Test an image header can be inserted at the image start""" + data = self.data = self._DoReadFileRealDtb('117_fdtmap_hdr_start.dts') + fdtmap_pos = 0x100 + len(U_BOOT_DATA) + hdr_data = data[:8] + self.assertEqual('BinM', hdr_data[:4]) + offset = struct.unpack('<I', hdr_data[4:])[0] + self.assertEqual(fdtmap_pos, offset) + + def testFdtmapHeaderPos(self): + """Test an image header can be inserted at a chosen position""" + data = self.data = self._DoReadFileRealDtb('118_fdtmap_hdr_pos.dts') + fdtmap_pos = 0x100 + len(U_BOOT_DATA) + hdr_data = data[0x80:0x88] + self.assertEqual('BinM', hdr_data[:4]) + offset = struct.unpack('<I', hdr_data[4:])[0] + self.assertEqual(fdtmap_pos, offset) + + def testHeaderMissingFdtmap(self): + """Test an image header requires an fdtmap""" + with self.assertRaises(ValueError) as e: + self.data = self._DoReadFileRealDtb('119_fdtmap_hdr_missing.dts') + self.assertIn("'image_header' section must have an 'fdtmap' sibling", + str(e.exception)) + + def testHeaderNoLocation(self): + """Test an image header with a no specified location is detected""" + with self.assertRaises(ValueError) as e: + self.data = self._DoReadFileRealDtb('120_hdr_no_location.dts') + self.assertIn("Invalid location 'None', expected 'start' or 'end'", + str(e.exception)) +

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/116_fdtmap_hdr.dts b/tools/binman/test/116_fdtmap_hdr.dts new file mode 100644 index 00000000000..77a2194b394 --- /dev/null +++ b/tools/binman/test/116_fdtmap_hdr.dts @@ -0,0 +1,17 @@ +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + size = <0x400>; + u-boot { + }; + fdtmap { + }; + image-header { + location = "end"; + }; + }; +}; diff --git a/tools/binman/test/117_fdtmap_hdr_start.dts b/tools/binman/test/117_fdtmap_hdr_start.dts new file mode 100644 index 00000000000..17b6be00470 --- /dev/null +++ b/tools/binman/test/117_fdtmap_hdr_start.dts @@ -0,0 +1,19 @@ +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + size = <0x400>; + sort-by-offset; + u-boot { + offset = <0x100>; + }; + fdtmap { + }; + image-header { + location = "start"; + }; + }; +}; diff --git a/tools/binman/test/118_fdtmap_hdr_pos.dts b/tools/binman/test/118_fdtmap_hdr_pos.dts new file mode 100644 index 00000000000..fd803f57fba --- /dev/null +++ b/tools/binman/test/118_fdtmap_hdr_pos.dts @@ -0,0 +1,19 @@ +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + size = <0x400>; + sort-by-offset; + u-boot { + offset = <0x100>; + }; + fdtmap { + }; + image-header { + offset = <0x80>; + }; + }; +}; diff --git a/tools/binman/test/119_fdtmap_hdr_missing.dts b/tools/binman/test/119_fdtmap_hdr_missing.dts new file mode 100644 index 00000000000..41bb680f08f --- /dev/null +++ b/tools/binman/test/119_fdtmap_hdr_missing.dts @@ -0,0 +1,16 @@ +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + sort-by-offset; + u-boot { + }; + image-header { + offset = <0x80>; + location = "start"; + }; + }; +}; diff --git a/tools/binman/test/120_hdr_no_location.dts b/tools/binman/test/120_hdr_no_location.dts new file mode 100644 index 00000000000..585e21f456b --- /dev/null +++ b/tools/binman/test/120_hdr_no_location.dts @@ -0,0 +1,16 @@ +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + sort-by-offset; + u-boot { + }; + fdtmap { + }; + image-header { + }; + }; +};

This class is the new way to handle arguments in Python. Convert binman over to use it. At the same time, introduce commands so that we can separate out the different parts of binman functionality.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: - Don't allow global arguments after the command - Fix up arguments in main Makefile - Pass verbosity to tests via the setup_test_args() method - Put --toolpath in the right place in functional tests

.travis.yml | 2 +- Makefile | 4 +- test/run | 4 +- tools/binman/README | 8 ++-- tools/binman/binman.py | 47 +++++++++++----------- tools/binman/cmdline.py | 82 +++++++++++++++++++++------------------ tools/binman/control.py | 51 ++++++++++++------------ tools/binman/ftest.py | 66 +++++++++++++------------------ tools/patman/test_util.py | 5 ++- 9 files changed, 133 insertions(+), 136 deletions(-)

diff --git a/.travis.yml b/.travis.yml index 70d89d3e233..2f604152090 100644 --- a/.travis.yml +++ b/.travis.yml @@ -147,7 +147,7 @@ script: if [[ -n "${TEST_PY_TOOLS}" ]]; then PYTHONPATH="${UBOOT_TRAVIS_BUILD_DIR}/scripts/dtc/pylibfdt" PATH="${UBOOT_TRAVIS_BUILD_DIR}/scripts/dtc:${PATH}" - ./tools/binman/binman --toolpath ${UBOOT_TRAVIS_BUILD_DIR}/tools -t && + ./tools/binman/binman --toolpath ${UBOOT_TRAVIS_BUILD_DIR}/tools test && ./tools/patman/patman --test && ./tools/buildman/buildman -t && PYTHONPATH="${UBOOT_TRAVIS_BUILD_DIR}/scripts/dtc/pylibfdt" diff --git a/Makefile b/Makefile index f3857ab6a9b..ee0d3c90029 100644 --- a/Makefile +++ b/Makefile @@ -1196,9 +1196,9 @@ u-boot.ldr: u-boot # --------------------------------------------------------------------------- # Use 'make BINMAN_DEBUG=1' to enable debugging quiet_cmd_binman = BINMAN $@ -cmd_binman = $(srctree)/tools/binman/binman -u -d u-boot.dtb -O . -m \ +cmd_binman = $(srctree)/tools/binman/binman build -u -d u-boot.dtb -O . -m \ -I . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \ - $(if $(BINMAN_DEBUG),-D) $(BINMAN_$(@F)) $< + $(if $(BINMAN_DEBUG),-D) $(BINMAN_$(@F))

OBJCOPYFLAGS_u-boot.ldr.hex := -I binary -O ihex

diff --git a/test/run b/test/run index b97647eba6f..d635622c106 100755 --- a/test/run +++ b/test/run @@ -40,7 +40,7 @@ export PYTHONPATH=${DTC_DIR}/pylibfdt export DTC=${DTC_DIR}/dtc TOOLS_DIR=build-sandbox_spl/tools

-run_test "binman" ./tools/binman/binman -t --toolpath ${TOOLS_DIR} +run_test "binman" ./tools/binman/binman --toolpath ${TOOLS_DIR} test run_test "patman" ./tools/patman/patman --test

[ "$1" == "quick" ] && skip=--skip-net-tests @@ -52,7 +52,7 @@ run_test "dtoc" ./tools/dtoc/dtoc -t # To enable Python test coverage on Debian-type distributions (e.g. Ubuntu): # $ sudo apt-get install python-pytest python-coverage export PATH=$PATH:${TOOLS_DIR} -run_test "binman code coverage" ./tools/binman/binman -T --toolpath ${TOOLS_DIR} +run_test "binman code coverage" ./tools/binman/binman test -T run_test "dtoc code coverage" ./tools/dtoc/dtoc -T run_test "fdt code coverage" ./tools/dtoc/test_fdt -T

diff --git a/tools/binman/README b/tools/binman/README index ef62d1f1ec7..fe734c1e5f7 100644 --- a/tools/binman/README +++ b/tools/binman/README @@ -187,7 +187,7 @@ First install prerequisites, e.g.

Type:

- binman -b <board_name> + binman build -b <board_name>

to build an image for a board. The board name is the same name used when configuring U-Boot (e.g. for sandbox_defconfig the board name is 'sandbox'). @@ -195,7 +195,7 @@ Binman assumes that the input files for the build are in ../b/<board_name>.

Or you can specify this explicitly:

- binman -I <build_path> + binman build -I <build_path>

where <build_path> is the build directory containing the output of the U-Boot build. @@ -483,7 +483,7 @@ Entry Documentation For details on the various entry types supported by binman and how to use them, see README.entries. This is generated from the source code using:

- binman -E >tools/binman/README.entries + binman entry-docs >tools/binman/README.entries

Hashing Entries @@ -715,7 +715,7 @@ Code coverage -------------

Binman is a critical tool and is designed to be very testable. Entry -implementations target 100% test coverage. Run 'binman -T' to check this. +implementations target 100% test coverage. Run 'binman test -T' to check this.

To enable Python test coverage on Debian-type distributions (e.g. Ubuntu):

diff --git a/tools/binman/binman.py b/tools/binman/binman.py index 613aad5c451..8bd5868df26 100755 --- a/tools/binman/binman.py +++ b/tools/binman/binman.py @@ -20,14 +20,15 @@ import sys import traceback import unittest

-# Bring in the patman and dtoc libraries +# Bring in the patman and dtoc libraries (but don't override the first path +# in PYTHONPATH) our_path = os.path.dirname(os.path.realpath(__file__)) for dirname in ['../patman', '../dtoc', '..', '../concurrencytest']: - sys.path.insert(0, os.path.join(our_path, dirname)) + sys.path.insert(2, os.path.join(our_path, dirname))

# Bring in the libfdt module -sys.path.insert(0, 'scripts/dtc/pylibfdt') -sys.path.insert(0, os.path.join(our_path, +sys.path.insert(2, 'scripts/dtc/pylibfdt') +sys.path.insert(2, os.path.join(our_path, '../../build-sandbox_spl/scripts/dtc/pylibfdt'))

# When running under python-coverage on Ubuntu 16.04, the dist-packages @@ -59,7 +60,7 @@ def RunTests(debug, verbosity, processes, test_preserve_dirs, args, toolpath): on the command line. processes: Number of processes to use to run tests (None=same as #CPUs) args: List of positional args provided to binman. This can hold a test - name to execute (as in 'binman -t testSections', for example) + name to execute (as in 'binman test testSections', for example) toolpath: List of paths to use for tools """ import cbfs_util_test @@ -98,7 +99,7 @@ def RunTests(debug, verbosity, processes, test_preserve_dirs, args, toolpath): setup_test_args = getattr(module, 'setup_test_args') setup_test_args(preserve_indir=test_preserve_dirs, preserve_outdirs=test_preserve_dirs and test_name is not None, - toolpath=toolpath) + toolpath=toolpath, verbosity=verbosity) if test_name: try: suite.addTests(loader.loadTestsFromName(test_name, module)) @@ -158,37 +159,36 @@ def RunTestCoverage(): for item in glob_list if '_testing' not in item]) test_util.RunTestCoverage('tools/binman/binman.py', None, ['*test*', '*binman.py', 'tools/patman/*', 'tools/dtoc/*'], - options.build_dir, all_set) + args.build_dir, all_set)

-def RunBinman(options, args): +def RunBinman(args): """Main entry point to binman once arguments are parsed

Args: - options: Command-line options - args: Non-option arguments + args: Command line arguments Namespace object """ ret_code = 0

- if not options.debug: + if not args.debug: sys.tracebacklimit = 0

- if options.test: - ret_code = RunTests(options.debug, options.verbosity, options.processes, - options.test_preserve_dirs, args[1:], - options.toolpath) - - elif options.test_coverage: - RunTestCoverage() + if args.cmd == 'test': + if args.test_coverage: + RunTestCoverage() + else: + ret_code = RunTests(args.debug, args.verbosity, args.processes, + args.test_preserve_dirs, args.tests, + args.toolpath)

- elif options.entry_docs: + elif args.cmd == 'entry-docs': control.WriteEntryDocs(GetEntryModules())

else: try: - ret_code = control.Binman(options, args) + ret_code = control.Binman(args) except Exception as e: print('binman: %s' % e) - if options.debug: + if args.debug: print() traceback.print_exc() ret_code = 1 @@ -196,6 +196,7 @@ def RunBinman(options, args):

if __name__ == "__main__": - (options, args) = cmdline.ParseArgs(sys.argv) - ret_code = RunBinman(options, args) + args = cmdline.ParseArgs(sys.argv[1:]) + + ret_code = RunBinman(args) sys.exit(ret_code) diff --git a/tools/binman/cmdline.py b/tools/binman/cmdline.py index 91e007e4e03..a002105fc77 100644 --- a/tools/binman/cmdline.py +++ b/tools/binman/cmdline.py @@ -5,7 +5,7 @@ # Command-line parser for binman #

-from optparse import OptionParser +from argparse import ArgumentParser

def ParseArgs(argv): """Parse the binman command-line arguments @@ -17,56 +17,64 @@ def ParseArgs(argv): options provides access to the options (e.g. option.debug) args is a list of string arguments """ - parser = OptionParser() - parser.add_option('-a', '--entry-arg', type='string', action='append', + if '-H' in argv: + argv.append('build') + + epilog = '''Binman creates and manipulate images for a board from a set of binaries. Binman is +controlled by a description in the board device tree.''' + + parser = ArgumentParser(epilog=epilog) + parser.add_argument('-B', '--build-dir', type=str, default='b', + help='Directory containing the build output') + parser.add_argument('-D', '--debug', action='store_true', + help='Enabling debugging (provides a full traceback on error)') + parser.add_argument('-H', '--full-help', action='store_true', + default=False, help='Display the README file') + parser.add_argument('--toolpath', type=str, action='append', + help='Add a path to the directories containing tools') + parser.add_argument('-v', '--verbosity', default=1, + type=int, help='Control verbosity: 0=silent, 1=warnings, 2=notices, ' + '3=info, 4=detail, 5=debug') + + subparsers = parser.add_subparsers(dest='cmd') + + build_parser = subparsers.add_parser('build', help='Build firmware image') + build_parser.add_argument('-a', '--entry-arg', type=str, action='append', help='Set argument value arg=value') - parser.add_option('-b', '--board', type='string', + build_parser.add_argument('-b', '--board', type=str, help='Board name to build') - parser.add_option('-B', '--build-dir', type='string', default='b', - help='Directory containing the build output') - parser.add_option('-d', '--dt', type='string', + build_parser.add_argument('-d', '--dt', type=str, help='Configuration file (.dtb) to use') - parser.add_option('-D', '--debug', action='store_true', - help='Enabling debugging (provides a full traceback on error)') - parser.add_option('-E', '--entry-docs', action='store_true', - help='Write out entry documentation (see README.entries)') - parser.add_option('--fake-dtb', action='store_true', + build_parser.add_argument('--fake-dtb', action='store_true', help='Use fake device tree contents (for testing only)') - parser.add_option('-i', '--image', type='string', action='append', + build_parser.add_argument('-i', '--image', type=str, action='append', help='Image filename to build (if not specified, build all)') - parser.add_option('-I', '--indir', action='append', + build_parser.add_argument('-I', '--indir', action='append', help='Add a path to the list of directories to use for input files') - parser.add_option('-H', '--full-help', action='store_true', - default=False, help='Display the README file') - parser.add_option('-m', '--map', action='store_true', + build_parser.add_argument('-m', '--map', action='store_true', default=False, help='Output a map file for each image') - parser.add_option('-O', '--outdir', type='string', + build_parser.add_argument('-O', '--outdir', type=str, action='store', help='Path to directory to use for intermediate and ' 'output files') - parser.add_option('-p', '--preserve', action='store_true',\ + build_parser.add_argument('-p', '--preserve', action='store_true',\ help='Preserve temporary output directory even if option -O is not ' 'given') - parser.add_option('-P', '--processes', type=int, - help='set number of processes to use for running tests') - parser.add_option('-t', '--test', action='store_true', - default=False, help='run tests') - parser.add_option('-T', '--test-coverage', action='store_true', - default=False, help='run tests and check for 100% coverage') - parser.add_option('--toolpath', type='string', action='append', - help='Add a path to the directories containing tools') - parser.add_option('-u', '--update-fdt', action='store_true', + build_parser.add_argument('-u', '--update-fdt', action='store_true', default=False, help='Update the binman node with offset/size info') - parser.add_option('-v', '--verbosity', default=1, - type='int', help='Control verbosity: 0=silent, 1=progress, 3=full, ' - '4=debug') - parser.add_option('-X', '--test-preserve-dirs', action='store_true', + + entry_parser = subparsers.add_parser('entry-docs', + help='Write out entry documentation (see README.entries)') + + test_parser = subparsers.add_parser('test', help='Run tests') + test_parser.add_argument('-P', '--processes', type=int, + help='set number of processes to use for running tests') + test_parser.add_argument('-T', '--test-coverage', action='store_true', + default=False, help='run tests and check for 100%% coverage') + test_parser.add_argument('-X', '--test-preserve-dirs', action='store_true', help='Preserve and display test-created input directories; also ' 'preserve the output directory if a single test is run (pass test ' 'name at the end of the command line') - - parser.usage += """ - -Create images for a board from a set of binaries. It is controlled by a -description in the board device tree.""" + test_parser.add_argument('tests', nargs='*', + help='Test names to run (omit for all)')

return parser.parse_args(argv) diff --git a/tools/binman/control.py b/tools/binman/control.py index 4a94afc8640..9022cf76e99 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -67,19 +67,18 @@ def WriteEntryDocs(modules, test_missing=None): from entry import Entry Entry.WriteDocs(modules, test_missing)

-def Binman(options, args): +def Binman(args): """The main control code for binman

This assumes that help and test options have already been dealt with. It deals with the core task of building images.

Args: - options: Command line options object - args: Command line arguments (list of strings) + args: Command line arguments Namespace object """ global images

- if options.full_help: + if args.full_help: pager = os.getenv('PAGER') if not pager: pager = 'more' @@ -89,17 +88,17 @@ def Binman(options, args): return 0

# Try to figure out which device tree contains our image description - if options.dt: - dtb_fname = options.dt + if args.dt: + dtb_fname = args.dt else: - board = options.board + board = args.board if not board: raise ValueError('Must provide a board to process (use -b <board>)') - board_pathname = os.path.join(options.build_dir, board) + board_pathname = os.path.join(args.build_dir, board) dtb_fname = os.path.join(board_pathname, 'u-boot.dtb') - if not options.indir: - options.indir = ['.'] - options.indir.append(board_pathname) + if not args.indir: + args.indir = ['.'] + args.indir.append(board_pathname)

try: # Import these here in case libfdt.py is not available, in which case @@ -107,15 +106,15 @@ def Binman(options, args): import fdt import fdt_util

- tout.Init(options.verbosity) - elf.debug = options.debug - cbfs_util.VERBOSE = options.verbosity > 2 - state.use_fake_dtb = options.fake_dtb + tout.Init(args.verbosity) + elf.debug = args.debug + cbfs_util.VERBOSE = args.verbosity > 2 + state.use_fake_dtb = args.fake_dtb try: - tools.SetInputDirs(options.indir) - tools.PrepareOutputDir(options.outdir, options.preserve) - tools.SetToolPaths(options.toolpath) - state.SetEntryArgs(options.entry_arg) + tools.SetInputDirs(args.indir) + tools.PrepareOutputDir(args.outdir, args.preserve) + tools.SetToolPaths(args.toolpath) + state.SetEntryArgs(args.entry_arg)

# Get the device tree ready by compiling it and copying the compiled # output into a file in our output directly. Then scan it for use @@ -132,16 +131,16 @@ def Binman(options, args):

images = _ReadImageDesc(node)

- if options.image: + if args.image: skip = [] new_images = OrderedDict() for name, image in images.items(): - if name in options.image: + if name in args.image: new_images[name] = image else: skip.append(name) images = new_images - if skip and options.verbosity >= 2: + if skip and args.verbosity >= 2: print('Skipping images: %s' % ', '.join(skip))

state.Prepare(images, dtb) @@ -155,7 +154,7 @@ def Binman(options, args): # entry offsets remain the same. for image in images.values(): image.ExpandEntries() - if options.update_fdt: + if args.update_fdt: image.AddMissingProperties() image.ProcessFdt(dtb)

@@ -176,19 +175,19 @@ def Binman(options, args): image.CheckSize() image.CheckEntries() except Exception as e: - if options.map: + if args.map: fname = image.WriteMap() print("Wrote map file '%s' to show errors" % fname) raise image.SetImagePos() - if options.update_fdt: + if args.update_fdt: image.SetCalculatedProperties() for dtb_item in state.GetFdts(): dtb_item.Sync() image.ProcessEntryContents() image.WriteSymbols() image.BuildImage() - if options.map: + if args.map: image.WriteMap()

# Write the updated FDTs to our output files diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 6fdecb2f3bf..c675299e1da 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -155,7 +155,7 @@ class TestFunctional(unittest.TestCase):

@classmethod def setup_test_args(cls, preserve_indir=False, preserve_outdirs=False, - toolpath=None): + toolpath=None, verbosity=None): """Accept arguments controlling test execution

Args: @@ -169,6 +169,7 @@ class TestFunctional(unittest.TestCase): cls.preserve_indir = preserve_indir cls.preserve_outdirs = preserve_outdirs cls.toolpath = toolpath + cls.verbosity = verbosity

def _CheckLz4(self): if not self.have_lz4: @@ -192,19 +193,6 @@ class TestFunctional(unittest.TestCase): TestFunctional._MakeInputFile('spl/u-boot-spl.dtb', U_BOOT_SPL_DTB_DATA) TestFunctional._MakeInputFile('tpl/u-boot-tpl.dtb', U_BOOT_TPL_DTB_DATA)

- def _GetVerbosity(self): - """Check if verbosity should be enabled - - Returns: - list containing either: - - Verbosity flag (e.g. '-v2') if it is present on the cmd line - - nothing if the flag is not present - """ - for arg in sys.argv[1:]: - if arg.startswith('-v'): - return [arg] - return [] - def _RunBinman(self, *args, **kwargs): """Run binman using the command line

@@ -219,7 +207,7 @@ class TestFunctional(unittest.TestCase): result.stdout + result.stderr)) return result

- def _DoBinman(self, *args): + def _DoBinman(self, *argv): """Run binman using directly (in the same process)

Args: @@ -227,16 +215,14 @@ class TestFunctional(unittest.TestCase): Returns: Return value (0 for success) """ - args = list(args) - if '-D' in sys.argv: - args = args + ['-D'] - (options, args) = cmdline.ParseArgs(args) - options.pager = 'binman-invalid-pager' - options.build_dir = self._indir + argv = list(argv) + args = cmdline.ParseArgs(argv) + args.pager = 'binman-invalid-pager' + args.build_dir = self._indir

# For testing, you can force an increase in verbosity here - # options.verbosity = tout.DEBUG - return control.Binman(options, args) + # args.verbosity = tout.DEBUG + return control.Binman(args)

def _DoTestFile(self, fname, debug=False, map=False, update_dtb=False, entry_args=None, images=None, use_real_dtb=False, @@ -254,28 +240,29 @@ class TestFunctional(unittest.TestCase): value: value of that arg images: List of image names to build """ - args = ['-p', '-I', self._indir, '-d', self.TestFile(fname)] + args = [] if debug: args.append('-D') + if verbosity is not None: + args.append('-v%d' % verbosity) + elif self.verbosity: + args.append('-v%d' % self.verbosity) + if self.toolpath: + for path in self.toolpath: + args += ['--toolpath', path] + args += ['build', '-p', '-I', self._indir, '-d', self.TestFile(fname)] if map: args.append('-m') if update_dtb: args.append('-u') if not use_real_dtb: args.append('--fake-dtb') - if verbosity is not None: - args.append('-v%d' % verbosity) - else: - args += self._GetVerbosity() if entry_args: for arg, value in entry_args.items(): args.append('-a%s=%s' % (arg, value)) if images: for image in images: args += ['-i', image] - if self.toolpath: - for path in self.toolpath: - args += ['--toolpath', path] return self._DoBinman(*args)

def _SetupDtb(self, fname, outfile='u-boot.dtb'): @@ -538,20 +525,20 @@ class TestFunctional(unittest.TestCase): """Test that we can run it with a specific board""" self._SetupDtb('005_simple.dts', 'sandbox/u-boot.dtb') TestFunctional._MakeInputFile('sandbox/u-boot.bin', U_BOOT_DATA) - result = self._DoBinman('-b', 'sandbox') + result = self._DoBinman('build', '-b', 'sandbox') self.assertEqual(0, result)

def testNeedBoard(self): """Test that we get an error when no board ius supplied""" with self.assertRaises(ValueError) as e: - result = self._DoBinman() + result = self._DoBinman('build') self.assertIn("Must provide a board to process (use -b <board>)", str(e.exception))

def testMissingDt(self): """Test that an invalid device-tree file generates an error""" with self.assertRaises(Exception) as e: - self._RunBinman('-d', 'missing_file') + self._RunBinman('build', '-d', 'missing_file') # We get one error from libfdt, and a different one from fdtget. self.AssertInList(["Couldn't open blob from 'missing_file'", 'No such file or directory'], str(e.exception)) @@ -563,26 +550,26 @@ class TestFunctional(unittest.TestCase): will come from the device-tree compiler (dtc). """ with self.assertRaises(Exception) as e: - self._RunBinman('-d', self.TestFile('001_invalid.dts')) + self._RunBinman('build', '-d', self.TestFile('001_invalid.dts')) self.assertIn("FATAL ERROR: Unable to parse input tree", str(e.exception))

def testMissingNode(self): """Test that a device tree without a 'binman' node generates an error""" with self.assertRaises(Exception) as e: - self._DoBinman('-d', self.TestFile('002_missing_node.dts')) + self._DoBinman('build', '-d', self.TestFile('002_missing_node.dts')) self.assertIn("does not have a 'binman' node", str(e.exception))

def testEmpty(self): """Test that an empty binman node works OK (i.e. does nothing)""" - result = self._RunBinman('-d', self.TestFile('003_empty.dts')) + result = self._RunBinman('build', '-d', self.TestFile('003_empty.dts')) self.assertEqual(0, len(result.stderr)) self.assertEqual(0, result.return_code)

def testInvalidEntry(self): """Test that an invalid entry is flagged""" with self.assertRaises(Exception) as e: - result = self._RunBinman('-d', + result = self._RunBinman('build', '-d', self.TestFile('004_invalid_entry.dts')) self.assertIn("Unknown entry type 'not-a-valid-type' in node " "'/binman/not-a-valid-type'", str(e.exception)) @@ -1313,7 +1300,8 @@ class TestFunctional(unittest.TestCase):

def testEntryArgsInvalidFormat(self): """Test that an invalid entry-argument format is detected""" - args = ['-d', self.TestFile('064_entry_args_required.dts'), '-ano-value'] + args = ['build', '-d', self.TestFile('064_entry_args_required.dts'), + '-ano-value'] with self.assertRaises(ValueError) as e: self._DoBinman(*args) self.assertIn("Invalid entry arguemnt 'no-value'", str(e.exception)) diff --git a/tools/patman/test_util.py b/tools/patman/test_util.py index ea36cd16339..40098159c08 100644 --- a/tools/patman/test_util.py +++ b/tools/patman/test_util.py @@ -46,9 +46,10 @@ def RunTestCoverage(prog, filter_fname, exclude_list, build_dir, required=None): glob_list = [] glob_list += exclude_list glob_list += ['*libfdt.py', '*site-packages*', '*dist-packages*'] + test_cmd = 'test' if 'binman.py' in prog else '-t' cmd = ('PYTHONPATH=$PYTHONPATH:%s/sandbox_spl/tools %s-coverage run ' - '--omit "%s" %s -P1 -t' % (build_dir, PYTHON, ','.join(glob_list), - prog)) + '--omit "%s" %s %s -P1' % (build_dir, PYTHON, ','.join(glob_list), + prog, test_cmd)) os.system(cmd) stdout = command.Output('%s-coverage' % PYTHON, 'report') lines = stdout.splitlines()

Compression is currently available only with blobs. However we want to report the compression algorithm and uncompressed size for all entries, so that other entry types can support compression. This will help with the forthcoming 'list' feature which lists entries in the image.

Move the compression properties into the base class. Also fix up the docs which had the wrong property name.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/README | 11 ++++++++--- tools/binman/entry.py | 9 +++++++++ tools/binman/etype/blob.py | 19 ++++--------------- 3 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/tools/binman/README b/tools/binman/README index fe734c1e5f7..abbf809b823 100644 --- a/tools/binman/README +++ b/tools/binman/README @@ -339,6 +339,10 @@ expand-size: limited by the size of the image/section and the position of the next entry.

+compress: + Sets the compression algortihm to use (for blobs only). See the entry + documentation for details. + The attributes supported for images and sections are described below. Several are similar to those for entries.

@@ -649,15 +653,16 @@ Compression -----------

Binman support compression for 'blob' entries (those of type 'blob' and -derivatives). To enable this for an entry, add a 'compression' property: +derivatives). To enable this for an entry, add a 'compress' property:

blob { filename = "datafile"; - compression = "lz4"; + compress = "lz4"; };

The entry will then contain the compressed data, using the 'lz4' compression -algorithm. Currently this is the only one that is supported. +algorithm. Currently this is the only one that is supported. The uncompressed +size is written to the node in an 'uncomp-size' property, if -u is used.

diff --git a/tools/binman/entry.py b/tools/binman/entry.py index e1cd0d3a882..8cccc2ed5f0 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -51,6 +51,8 @@ class Entry(object): offset: Offset of entry within the section, None if not known yet (in which case it will be calculated by Pack()) size: Entry size in bytes, None if not known + uncomp_size: Size of uncompressed data in bytes, if the entry is + compressed, else None contents_size: Size of contents in bytes, 0 by default align: Entry start offset alignment, or None align_size: Entry size alignment, or None @@ -58,6 +60,7 @@ class Entry(object): pad_before: Number of pad bytes before the contents, 0 if none pad_after: Number of pad bytes after the contents, 0 if none data: Contents of entry (string of bytes) + compress: Compression algoithm used (e.g. 'lz4'), 'none' if none """ def __init__(self, section, etype, node, read_node=True, name_prefix=''): self.section = section @@ -66,6 +69,7 @@ class Entry(object): self.name = node and (name_prefix + node.name) or 'none' self.offset = None self.size = None + self.uncomp_size = None self.data = None self.contents_size = 0 self.align = None @@ -76,6 +80,7 @@ class Entry(object): self.offset_unset = False self.image_pos = None self._expand_size = False + self.compress = 'none' if read_node: self.ReadNode()

@@ -188,6 +193,8 @@ class Entry(object): for prop in ['offset', 'size', 'image-pos']: if not prop in self._node.props: state.AddZeroProp(self._node, prop) + if self.compress != 'none': + state.AddZeroProp(self._node, 'uncomp-size') err = state.CheckAddHashProp(self._node) if err: self.Raise(err) @@ -198,6 +205,8 @@ class Entry(object): state.SetInt(self._node, 'size', self.size) state.SetInt(self._node, 'image-pos', self.image_pos - self.section.GetRootSkipAtStart()) + if self.uncomp_size is not None: + state.SetInt(self._node, 'uncomp-size', self.uncomp_size) state.CheckSetHashValue(self._node, self.GetData)

def ProcessFdt(self, fdt): diff --git a/tools/binman/etype/blob.py b/tools/binman/etype/blob.py index a91e7847009..ec94568ac0a 100644 --- a/tools/binman/etype/blob.py +++ b/tools/binman/etype/blob.py @@ -33,8 +33,7 @@ class Entry_blob(Entry): def __init__(self, section, etype, node): Entry.__init__(self, section, etype, node) self._filename = fdt_util.GetString(self._node, 'filename', self.etype) - self._compress = fdt_util.GetString(self._node, 'compress', 'none') - self._uncompressed_size = None + self.compress = fdt_util.GetString(self._node, 'compress', 'none')

def ObtainContents(self): self._filename = self.GetDefaultFilename() @@ -50,21 +49,11 @@ class Entry_blob(Entry): # the data in chunks and avoid reading it all at once. For now # this seems like an unnecessary complication. indata = tools.ReadFile(self._pathname) - if self._compress != 'none': - self._uncompressed_size = len(indata) - data = tools.Compress(indata, self._compress) + if self.compress != 'none': + self.uncomp_size = len(indata) + data = tools.Compress(indata, self.compress) self.SetContents(data) return True

def GetDefaultFilename(self): return self._filename - - def AddMissingProperties(self): - Entry.AddMissingProperties(self) - if self._compress != 'none': - state.AddZeroProp(self._node, 'uncomp-size') - - def SetCalculatedProperties(self): - Entry.SetCalculatedProperties(self) - if self._uncompressed_size is not None: - state.SetInt(self._node, 'uncomp-size', self._uncompressed_size)

The first argument is not used. Remove it.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/entry.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 8cccc2ed5f0..00bb1d190a9 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -85,11 +85,10 @@ class Entry(object): self.ReadNode()

@staticmethod - def Lookup(section, node_path, etype): + def Lookup(node_path, etype): """Look up the entry class for a node.

Args: - section: Section object containing this node node_node: Path name of Node object containing information about the entry to create (used for errors) etype: Entry type to use @@ -140,7 +139,7 @@ class Entry(object): """ if not etype: etype = fdt_util.GetString(node, 'type', node.name) - obj = Entry.Lookup(section, node.path, etype) + obj = Entry.Lookup(node.path, etype)

# Call its constructor to get the object we want. return obj(section, etype, node) @@ -514,7 +513,7 @@ features to produce new behaviours. modules.remove('_testing') missing = [] for name in modules: - module = Entry.Lookup(name, name, name) + module = Entry.Lookup(name, name) docs = getattr(module, '__doc__') if test_missing == name: docs = None

At present entry modules can only be accessed using Entry.Lookup() or Entry.Create(). Most of the time this is fine, but sometimes a module needs to provide constants or helper functions useful to other modules. It is easier in this case to use 'import'.

Add an __init__ file to permit this.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/entry.py | 2 ++ tools/binman/etype/__init__.py | 0 tools/patman/test_util.py | 1 + 3 files changed, 3 insertions(+) create mode 100644 tools/binman/etype/__init__.py

diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 00bb1d190a9..a04e149d96c 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -513,6 +513,8 @@ features to produce new behaviours. modules.remove('_testing') missing = [] for name in modules: + if name.startswith('__'): + continue module = Entry.Lookup(name, name) docs = getattr(module, '__doc__') if test_missing == name: diff --git a/tools/binman/etype/__init__.py b/tools/binman/etype/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tools/patman/test_util.py b/tools/patman/test_util.py index 40098159c08..09f258c26b4 100644 --- a/tools/patman/test_util.py +++ b/tools/patman/test_util.py @@ -58,6 +58,7 @@ def RunTestCoverage(prog, filter_fname, exclude_list, build_dir, required=None): test_set = set([os.path.splitext(os.path.basename(line.split()[0]))[0] for line in lines if '/etype/' in line]) missing_list = required + missing_list.discard('__init__') missing_list.difference_update(test_set) if missing_list: print('Missing tests for %s' % (', '.join(missing_list)))

This function raises an exception with its arguments around the wrong way so the message is incorrect. Fix this as well as a few minor comment problems.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/entry.py | 8 ++++---- tools/binman/ftest.py | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/binman/entry.py b/tools/binman/entry.py index a04e149d96c..b19a3b026f2 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -237,25 +237,25 @@ class Entry(object): This sets both the data and content_size properties

Args: - data: Data to set to the contents (string) + data: Data to set to the contents (bytes) """ self.data = data self.contents_size = len(self.data)

def ProcessContentsUpdate(self, data): - """Update the contens of an entry, after the size is fixed + """Update the contents of an entry, after the size is fixed

This checks that the new data is the same size as the old.

Args: - data: Data to set to the contents (string) + data: Data to set to the contents (bytes)

Raises: ValueError if the new data size is not the same as the old """ if len(data) != self.contents_size: self.Raise('Cannot update entry size from %d to %d' % - (len(data), self.contents_size)) + (self.contents_size, len(data))) self.SetContents(data)

def ObtainContents(self): diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index c675299e1da..1c917345f2a 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -1223,7 +1223,7 @@ class TestFunctional(unittest.TestCase): with self.assertRaises(ValueError) as e: self._DoReadFile('059_change_size.dts', True) self.assertIn("Node '/binman/_testing': Cannot update entry size from " - '2 to 1', str(e.exception)) + '1 to 2', str(e.exception))

def testUpdateFdt(self): """Test that we can update the device tree with offset/size info"""

SetContents() should only be called to set the contents of an entry from within the ObtainContents() call, since it has no guard against increasing the size of the contents, thus triggering incorrect operation.

Change all such calls to use ProcessUpdateContents() instead.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/etype/blob_dtb.py | 2 +- tools/binman/etype/fdtmap.py | 2 +- tools/binman/etype/fmap.py | 2 +- tools/binman/etype/image_header.py | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/binman/etype/blob_dtb.py b/tools/binman/etype/blob_dtb.py index cc5b4a3f760..d80c3d7e006 100644 --- a/tools/binman/etype/blob_dtb.py +++ b/tools/binman/etype/blob_dtb.py @@ -30,4 +30,4 @@ class Entry_blob_dtb(Entry_blob): def ProcessContents(self): """Re-read the DTB contents so that we get any calculated properties""" _, data = state.GetFdtContents(self._filename) - self.SetContents(data) + self.ProcessContentsUpdate(data) diff --git a/tools/binman/etype/fdtmap.py b/tools/binman/etype/fdtmap.py index cdeb491ebdc..ddac148b9ba 100644 --- a/tools/binman/etype/fdtmap.py +++ b/tools/binman/etype/fdtmap.py @@ -106,4 +106,4 @@ class Entry_fdtmap(Entry): This is necessary since new data may have been written back to it during processing, e.g. the image-pos properties. """ - self.SetContents(self._GetFdtmap()) + self.ProcessContentsUpdate(self._GetFdtmap()) diff --git a/tools/binman/etype/fmap.py b/tools/binman/etype/fmap.py index e6b5c5c74c0..45d6db18a31 100644 --- a/tools/binman/etype/fmap.py +++ b/tools/binman/etype/fmap.py @@ -62,4 +62,4 @@ class Entry_fmap(Entry): return True

def ProcessContents(self): - self.SetContents(self._GetFmap()) + self.ProcessContentsUpdate(self._GetFmap()) diff --git a/tools/binman/etype/image_header.py b/tools/binman/etype/image_header.py index 9bc84ec01d4..d6de58ce4b7 100644 --- a/tools/binman/etype/image_header.py +++ b/tools/binman/etype/image_header.py @@ -73,4 +73,4 @@ class Entry_image_header(Entry): This is necessary since image_pos is not available when ObtainContents() is called, since by then the entries have not been packed in the image. """ - self.SetContents(self._GetHeader()) + self.ProcessContentsUpdate(self._GetHeader())

At present if this function tries to update the contents such that the size changes, it raises an error. We plan to add the ability to change the size of entries after packing is completed, since in some cases it is not possible to determine the size in advance.

An example of this is with a compressed device tree, where the values of the device tree change in SetCalculatedProperties() or ProcessEntryContents(). While the device tree itself does not change size, since placeholders for any new properties have already bee added by AddMissingProperties(), we cannot predict the size of the device tree after compression. If a value changes from 0 to 0x1234 (say), then the compressed device tree may expand.

As a first step towards supporting this, make ProcessContentsUpdate() return a value indicating whether the content size is OK. For now this is always True (since otherwise binman raises an error), but later patches will adjust this.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/bsection.py | 8 +++++++- tools/binman/entry.py | 22 +++++++++++++++++++-- tools/binman/etype/_testing.py | 5 +++-- tools/binman/etype/blob_dtb.py | 2 +- tools/binman/etype/fdtmap.py | 2 +- tools/binman/etype/fmap.py | 2 +- tools/binman/etype/image_header.py | 2 +- tools/binman/etype/section.py | 5 +++-- tools/binman/etype/u_boot_with_ucode_ptr.py | 6 +++--- tools/binman/image.py | 5 ++++- 10 files changed, 44 insertions(+), 15 deletions(-)

diff --git a/tools/binman/bsection.py b/tools/binman/bsection.py index 3e3d369d5e4..f49a6e93bc7 100644 --- a/tools/binman/bsection.py +++ b/tools/binman/bsection.py @@ -317,9 +317,15 @@ class Section(object): """Call the ProcessContents() method for each entry

This is intended to adjust the contents as needed by the entry type. + + Returns: + True if no entries needed to change their size """ + sizes_ok = True for entry in self._entries.values(): - entry.ProcessContents() + if not entry.ProcessContents(): + sizes_ok = False + return sizes_ok

def WriteSymbols(self): """Write symbol values into binary files for access at run time""" diff --git a/tools/binman/entry.py b/tools/binman/entry.py index b19a3b026f2..7db1402b84f 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -245,7 +245,8 @@ class Entry(object): def ProcessContentsUpdate(self, data): """Update the contents of an entry, after the size is fixed

- This checks that the new data is the same size as the old. + This checks that the new data is the same size as the old. If the size + has changed, this triggers a re-run of the packing algorithm.

Args: data: Data to set to the contents (bytes) @@ -253,10 +254,12 @@ class Entry(object): Raises: ValueError if the new data size is not the same as the old """ + size_ok = True if len(data) != self.contents_size: self.Raise('Cannot update entry size from %d to %d' % (self.contents_size, len(data))) self.SetContents(data) + return size_ok

def ObtainContents(self): """Figure out the contents of an entry. @@ -401,7 +404,22 @@ class Entry(object): self.image_pos = image_pos + self.offset

def ProcessContents(self): - pass + """Do any post-packing updates of entry contents + + This function should call ProcessContentsUpdate() to update the entry + contents, if necessary, returning its return value here. + + Args: + data: Data to set to the contents (bytes) + + Returns: + True if the new data size is OK, False if expansion is needed + + Raises: + ValueError if the new data size is not the same as the old and + state.AllowEntryExpansion() is False + """ + return True

def WriteSymbols(self, section): """Write symbol values into binary files for access at run time diff --git a/tools/binman/etype/_testing.py b/tools/binman/etype/_testing.py index ac62d2e2005..2204362281c 100644 --- a/tools/binman/etype/_testing.py +++ b/tools/binman/etype/_testing.py @@ -88,9 +88,10 @@ class Entry__testing(Entry):

def ProcessContents(self): if self.bad_update_contents: - # Request to update the conents with something larger, to cause a + # Request to update the contents with something larger, to cause a # failure. - self.ProcessContentsUpdate('aa') + return self.ProcessContentsUpdate('aa') + return True

def ProcessFdt(self, fdt): """Force reprocessing the first time""" diff --git a/tools/binman/etype/blob_dtb.py b/tools/binman/etype/blob_dtb.py index d80c3d7e006..09d5d727138 100644 --- a/tools/binman/etype/blob_dtb.py +++ b/tools/binman/etype/blob_dtb.py @@ -30,4 +30,4 @@ class Entry_blob_dtb(Entry_blob): def ProcessContents(self): """Re-read the DTB contents so that we get any calculated properties""" _, data = state.GetFdtContents(self._filename) - self.ProcessContentsUpdate(data) + return self.ProcessContentsUpdate(data) diff --git a/tools/binman/etype/fdtmap.py b/tools/binman/etype/fdtmap.py index ddac148b9ba..bfd7962be3a 100644 --- a/tools/binman/etype/fdtmap.py +++ b/tools/binman/etype/fdtmap.py @@ -106,4 +106,4 @@ class Entry_fdtmap(Entry): This is necessary since new data may have been written back to it during processing, e.g. the image-pos properties. """ - self.ProcessContentsUpdate(self._GetFdtmap()) + return self.ProcessContentsUpdate(self._GetFdtmap()) diff --git a/tools/binman/etype/fmap.py b/tools/binman/etype/fmap.py index 45d6db18a31..3a809486098 100644 --- a/tools/binman/etype/fmap.py +++ b/tools/binman/etype/fmap.py @@ -62,4 +62,4 @@ class Entry_fmap(Entry): return True

def ProcessContents(self): - self.ProcessContentsUpdate(self._GetFmap()) + return self.ProcessContentsUpdate(self._GetFmap()) diff --git a/tools/binman/etype/image_header.py b/tools/binman/etype/image_header.py index d6de58ce4b7..b1c4f8a07e9 100644 --- a/tools/binman/etype/image_header.py +++ b/tools/binman/etype/image_header.py @@ -73,4 +73,4 @@ class Entry_image_header(Entry): This is necessary since image_pos is not available when ObtainContents() is called, since by then the entries have not been packed in the image. """ - self.ProcessContentsUpdate(self._GetHeader()) + return self.ProcessContentsUpdate(self._GetHeader()) diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 3681a484689..51eddcd995a 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -85,8 +85,9 @@ class Entry_section(Entry): self._section.SetCalculatedProperties()

def ProcessContents(self): - self._section.ProcessEntryContents() - super(Entry_section, self).ProcessContents() + sizes_ok = self._section.ProcessEntryContents() + sizes_ok_base = super(Entry_section, self).ProcessContents() + return sizes_ok and sizes_ok_base

def CheckOffset(self): self._section.CheckEntries() diff --git a/tools/binman/etype/u_boot_with_ucode_ptr.py b/tools/binman/etype/u_boot_with_ucode_ptr.py index da0e12417b5..4104bf8bf13 100644 --- a/tools/binman/etype/u_boot_with_ucode_ptr.py +++ b/tools/binman/etype/u_boot_with_ucode_ptr.py @@ -91,6 +91,6 @@ class Entry_u_boot_with_ucode_ptr(Entry_blob): # Write the microcode offset and size into the entry offset_and_size = struct.pack('<2L', offset, size) self.target_offset -= self.image_pos - self.ProcessContentsUpdate(self.data[:self.target_offset] + - offset_and_size + - self.data[self.target_offset + 8:]) + return self.ProcessContentsUpdate(self.data[:self.target_offset] + + offset_and_size + + self.data[self.target_offset + 8:]) diff --git a/tools/binman/image.py b/tools/binman/image.py index f237ae302df..c8bce394aa1 100644 --- a/tools/binman/image.py +++ b/tools/binman/image.py @@ -122,8 +122,11 @@ class Image: """Call the ProcessContents() method for each entry

This is intended to adjust the contents as needed by the entry type. + + Returns: + True if the new data size is OK, False if expansion is needed """ - self._section.ProcessEntryContents() + return self._section.ProcessEntryContents()

def WriteSymbols(self): """Write symbol values into binary files for access at run time"""

We plan to support changing the size of entries after they have been packed. For now it will always be enabled. But to aid testing of both cases (in the event that we want to add a command-line flag, for example), add a setting to control it.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/state.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)

diff --git a/tools/binman/state.py b/tools/binman/state.py index 3ccd7855d47..382bda32219 100644 --- a/tools/binman/state.py +++ b/tools/binman/state.py @@ -31,6 +31,11 @@ fdt_subset = set() # The DTB which contains the full image information main_dtb = None

+# Allow entries to expand after they have been packed. This is detected and +# forces a re-pack. If not allowed, any attempted expansion causes an error in +# Entry.ProcessContentsUpdate() +allow_entry_expansion = True + def GetFdt(fname): """Get the Fdt object for a particular device-tree filename

@@ -250,3 +255,22 @@ def CheckSetHashValue(node, get_data_func): data = m.digest() for n in GetUpdateNodes(hash_node): n.SetData('value', data) + +def SetAllowEntryExpansion(allow): + """Set whether post-pack expansion of entries is allowed + + Args: + allow: True to allow expansion, False to raise an exception + """ + global allow_entry_expansion + + allow_entry_expansion = allow + +def AllowEntryExpansion(): + """Check whether post-pack expansion of entries is allowed + + Returns: + True if expansion should be allowed, False if an exception should be + raised + """ + return allow_entry_expansion

Add support for detecting entries that change size after they have already been packed, and re-running packing when it happens.

This removes the limitation that entry size cannot change after PackEntries() is called.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: - Avoid dual assignment in ResetForPack() - Show a message when a section's size changes - Use 3 packing passes instead of two, and add a comment - Use bytearray for Python 3

tools/binman/README | 3 +- tools/binman/bsection.py | 12 +++++ tools/binman/control.py | 51 +++++++++++++------ tools/binman/entry.py | 21 +++++++- tools/binman/etype/_testing.py | 11 +++- tools/binman/etype/section.py | 5 ++ tools/binman/etype/u_boot_with_ucode_ptr.py | 2 +- tools/binman/ftest.py | 33 ++++++++++-- tools/binman/image.py | 8 +++ tools/binman/test/121_entry_expand.dts | 20 ++++++++ tools/binman/test/122_entry_expand_twice.dts | 21 ++++++++ .../binman/test/123_entry_expand_section.dts | 22 ++++++++ 12 files changed, 184 insertions(+), 25 deletions(-) create mode 100644 tools/binman/test/121_entry_expand.dts create mode 100644 tools/binman/test/122_entry_expand_twice.dts create mode 100644 tools/binman/test/123_entry_expand_section.dts

diff --git a/tools/binman/README b/tools/binman/README index abbf809b823..9860633792f 100644 --- a/tools/binman/README +++ b/tools/binman/README @@ -566,7 +566,8 @@ tree. This sets the correct 'offset' and 'size' vaues, for example. The default implementatoin does nothing. This can be overriden to adjust the contents of an entry in some way. For example, it would be possible to create an entry containing a hash of the contents of some other entries. At this -stage the offset and size of entries should not be adjusted. +stage the offset and size of entries should not be adjusted unless absolutely +necessary, since it requires a repack (going back to PackEntries()).

10. WriteSymbols() - write the value of symbols into the U-Boot SPL binary. See 'Access to binman entry offsets at run time' below for a description of diff --git a/tools/binman/bsection.py b/tools/binman/bsection.py index f49a6e93bc7..9047e55a34a 100644 --- a/tools/binman/bsection.py +++ b/tools/binman/bsection.py @@ -45,6 +45,8 @@ class Section(object): _name_prefix: Prefix to add to the name of all entries within this section _entries: OrderedDict() of entries + _orig_offset: Original offset value read from node + _orig_size: Original size value read from node """ def __init__(self, name, parent_section, node, image, test=False): global entry @@ -76,6 +78,8 @@ class Section(object): """Read properties from the section node""" self._offset = fdt_util.GetInt(self._node, 'offset') self._size = fdt_util.GetInt(self._node, 'size') + self._orig_offset = self._offset + self._orig_size = self._size self._align_size = fdt_util.GetInt(self._node, 'align-size') if tools.NotPowerOfTwo(self._align_size): self._Raise("Alignment size %s must be a power of two" % @@ -257,6 +261,13 @@ class Section(object): for name, info in offset_dict.items(): self._SetEntryOffsetSize(name, *info)

+ def ResetForPack(self): + """Reset offset/size fields so that packing can be done again""" + self._offset = self._orig_offset + self._size = self._orig_size + for entry in self._entries.values(): + entry.ResetForPack() + def PackEntries(self): """Pack all entries into the section""" offset = self._skip_at_start @@ -325,6 +336,7 @@ class Section(object): for entry in self._entries.values(): if not entry.ProcessContents(): sizes_ok = False + print("Entry '%s' size change" % self._node.path) return sizes_ok

def WriteSymbols(self): diff --git a/tools/binman/control.py b/tools/binman/control.py index 9022cf76e99..35faf115062 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -170,21 +170,42 @@ def Binman(args): # completed and written, but that does not seem important. image.GetEntryContents() image.GetEntryOffsets() - try: - image.PackEntries() - image.CheckSize() - image.CheckEntries() - except Exception as e: - if args.map: - fname = image.WriteMap() - print("Wrote map file '%s' to show errors" % fname) - raise - image.SetImagePos() - if args.update_fdt: - image.SetCalculatedProperties() - for dtb_item in state.GetFdts(): - dtb_item.Sync() - image.ProcessEntryContents() + + # We need to pack the entries to figure out where everything + # should be placed. This sets the offset/size of each entry. + # However, after packing we call ProcessEntryContents() which + # may result in an entry changing size. In that case we need to + # do another pass. Since the device tree often contains the + # final offset/size information we try to make space for this in + # AddMissingProperties() above. However, if the device is + # compressed we cannot know this compressed size in advance, + # since changing an offset from 0x100 to 0x104 (for example) can + # alter the compressed size of the device tree. So we need a + # third pass for this. + passes = 3 + for pack_pass in range(passes): + try: + image.PackEntries() + image.CheckSize() + image.CheckEntries() + except Exception as e: + if args.map: + fname = image.WriteMap() + print("Wrote map file '%s' to show errors" % fname) + raise + image.SetImagePos() + if args.update_fdt: + image.SetCalculatedProperties() + for dtb_item in state.GetFdts(): + dtb_item.Sync() + sizes_ok = image.ProcessEntryContents() + if sizes_ok: + break + image.ResetForPack() + if not sizes_ok: + image.Raise('Entries expanded after packing (tried %s passes)' % + passes) + image.WriteSymbols() image.BuildImage() if args.map: diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 7db1402b84f..e38cb71c596 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -61,6 +61,8 @@ class Entry(object): pad_after: Number of pad bytes after the contents, 0 if none data: Contents of entry (string of bytes) compress: Compression algoithm used (e.g. 'lz4'), 'none' if none + orig_offset: Original offset value read from node + orig_size: Original size value read from node """ def __init__(self, section, etype, node, read_node=True, name_prefix=''): self.section = section @@ -153,6 +155,9 @@ class Entry(object): self.Raise("Please use 'offset' instead of 'pos'") self.offset = fdt_util.GetInt(self._node, 'offset') self.size = fdt_util.GetInt(self._node, 'size') + self.orig_offset = self.offset + self.orig_size = self.size + self.align = fdt_util.GetInt(self._node, 'align') if tools.NotPowerOfTwo(self.align): raise ValueError("Node '%s': Alignment %s must be a power of two" % @@ -255,9 +260,16 @@ class Entry(object): ValueError if the new data size is not the same as the old """ size_ok = True - if len(data) != self.contents_size: + new_size = len(data) + if state.AllowEntryExpansion(): + if new_size > self.contents_size: + print("Entry '%s' size change from %#x to %#x" % ( + self._node.path, self.contents_size, new_size)) + # self.data will indicate the new size needed + size_ok = False + elif new_size != self.contents_size: self.Raise('Cannot update entry size from %d to %d' % - (self.contents_size, len(data))) + (self.contents_size, new_size)) self.SetContents(data) return size_ok

@@ -271,6 +283,11 @@ class Entry(object): # No contents by default: subclasses can implement this return True

+ def ResetForPack(self): + """Reset offset/size fields so that packing can be done again""" + self.offset = self.orig_offset + self.size = self.orig_size + def Pack(self, offset): """Figure out how to pack the entry into the section

diff --git a/tools/binman/etype/_testing.py b/tools/binman/etype/_testing.py index 2204362281c..ae24fe8105a 100644 --- a/tools/binman/etype/_testing.py +++ b/tools/binman/etype/_testing.py @@ -50,6 +50,8 @@ class Entry__testing(Entry): 'bad-update-contents') self.return_contents_once = fdt_util.GetBool(self._node, 'return-contents-once') + self.bad_update_contents_twice = fdt_util.GetBool(self._node, + 'bad-update-contents-twice')

# Set to True when the entry is ready to process the FDT. self.process_fdt_ready = False @@ -71,11 +73,12 @@ class Entry__testing(Entry): if self.force_bad_datatype: self.GetEntryArgsOrProps([EntryArg('test-bad-datatype-arg', bool)]) self.return_contents = True + self.contents = b'a'

def ObtainContents(self): if self.return_unknown_contents or not self.return_contents: return False - self.data = b'a' + self.data = self.contents self.contents_size = len(self.data) if self.return_contents_once: self.return_contents = False @@ -90,7 +93,11 @@ class Entry__testing(Entry): if self.bad_update_contents: # Request to update the contents with something larger, to cause a # failure. - return self.ProcessContentsUpdate('aa') + if self.bad_update_contents_twice: + self.contents += b'a' + else: + self.contents = b'aa' + return self.ProcessContentsUpdate(self.contents) return True

def ProcessFdt(self, fdt): diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 51eddcd995a..23bf22113d4 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -64,6 +64,11 @@ class Entry_section(Entry): self._section.GetEntryOffsets() return {}

+ def ResetForPack(self): + """Reset offset/size fields so that packing can be done again""" + self._section.ResetForPack() + Entry.ResetForPack(self) + def Pack(self, offset): """Pack all entries into the section""" self._section.PackEntries() diff --git a/tools/binman/etype/u_boot_with_ucode_ptr.py b/tools/binman/etype/u_boot_with_ucode_ptr.py index 4104bf8bf13..cb7dbc68dbb 100644 --- a/tools/binman/etype/u_boot_with_ucode_ptr.py +++ b/tools/binman/etype/u_boot_with_ucode_ptr.py @@ -49,7 +49,7 @@ class Entry_u_boot_with_ucode_ptr(Entry_blob): def ProcessContents(self): # If the image does not need microcode, there is nothing to do if not self.target_offset: - return + return True

# Get the offset of the microcode ucode_entry = self.section.FindEntryType('u-boot-ucode') diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 1c917345f2a..aae8dbc1b33 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -1220,10 +1220,14 @@ class TestFunctional(unittest.TestCase):

def testBadChangeSize(self): """Test that trying to change the size of an entry fails""" - with self.assertRaises(ValueError) as e: - self._DoReadFile('059_change_size.dts', True) - self.assertIn("Node '/binman/_testing': Cannot update entry size from " - '1 to 2', str(e.exception)) + try: + state.SetAllowEntryExpansion(False) + with self.assertRaises(ValueError) as e: + self._DoReadFile('059_change_size.dts', True) + self.assertIn("Node '/binman/_testing': Cannot update entry size from 1 to 2", + str(e.exception)) + finally: + state.SetAllowEntryExpansion(True)

def testUpdateFdt(self): """Test that we can update the device tree with offset/size info""" @@ -2117,6 +2121,27 @@ class TestFunctional(unittest.TestCase): self.assertIn("Invalid location 'None', expected 'start' or 'end'", str(e.exception))

+ def testEntryExpand(self): + """Test expanding an entry after it is packed""" + data = self._DoReadFile('121_entry_expand.dts') + self.assertEqual(b'aa', data[:2]) + self.assertEqual(U_BOOT_DATA, data[2:2 + len(U_BOOT_DATA)]) + self.assertEqual(b'aa', data[-2:]) + + def testEntryExpandBad(self): + """Test expanding an entry after it is packed, twice""" + with self.assertRaises(ValueError) as e: + self._DoReadFile('122_entry_expand_twice.dts') + self.assertIn("Image '/binman': Entries expanded after packing", + str(e.exception)) + + def testEntryExpandSection(self): + """Test expanding an entry within a section after it is packed""" + data = self._DoReadFile('123_entry_expand_section.dts') + self.assertEqual(b'aa', data[:2]) + self.assertEqual(U_BOOT_DATA, data[2:2 + len(U_BOOT_DATA)]) + self.assertEqual(b'aa', data[-2:]) +

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/image.py b/tools/binman/image.py index c8bce394aa1..6339d020e76 100644 --- a/tools/binman/image.py +++ b/tools/binman/image.py @@ -55,6 +55,10 @@ class Image: self._filename = filename self._section = bsection.Section('main-section', None, self._node, self)

+ def Raise(self, msg): + """Convenience function to raise an error referencing an image""" + raise ValueError("Image '%s': %s" % (self._node.path, msg)) + def GetFdtSet(self): """Get the set of device tree files used by this image""" return self._section.GetFdtSet() @@ -100,6 +104,10 @@ class Image: """ self._section.GetEntryOffsets()

+ def ResetForPack(self): + """Reset offset/size fields so that packing can be done again""" + self._section.ResetForPack() + def PackEntries(self): """Pack all entries into the image""" self._section.PackEntries() diff --git a/tools/binman/test/121_entry_expand.dts b/tools/binman/test/121_entry_expand.dts new file mode 100644 index 00000000000..ebb7816db90 --- /dev/null +++ b/tools/binman/test/121_entry_expand.dts @@ -0,0 +1,20 @@ +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + _testing { + bad-update-contents; + }; + + u-boot { + }; + + _testing2 { + type = "_testing"; + bad-update-contents; + }; + }; +}; diff --git a/tools/binman/test/122_entry_expand_twice.dts b/tools/binman/test/122_entry_expand_twice.dts new file mode 100644 index 00000000000..258cf859f4b --- /dev/null +++ b/tools/binman/test/122_entry_expand_twice.dts @@ -0,0 +1,21 @@ +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + _testing { + bad-update-contents; + bad-update-contents-twice; + }; + + u-boot { + }; + + _testing2 { + type = "_testing"; + bad-update-contents; + }; + }; +}; diff --git a/tools/binman/test/123_entry_expand_section.dts b/tools/binman/test/123_entry_expand_section.dts new file mode 100644 index 00000000000..046f7234348 --- /dev/null +++ b/tools/binman/test/123_entry_expand_section.dts @@ -0,0 +1,22 @@ +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + _testing { + bad-update-contents; + }; + + u-boot { + }; + + section { + _testing2 { + type = "_testing"; + bad-update-contents; + }; + }; + }; +};

At present the logic skips the blob class' handling of compression, so this is not supported with device tree entries. Fix this.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: - Update test to use _DoReadFileRealDtb() helper

tools/binman/etype/blob.py | 25 +++++++++++++++++-------- tools/binman/etype/blob_dtb.py | 8 ++++---- tools/binman/ftest.py | 18 ++++++++++++++++++ tools/binman/test/124_compress_dtb.dts | 14 ++++++++++++++ 4 files changed, 53 insertions(+), 12 deletions(-) create mode 100644 tools/binman/test/124_compress_dtb.dts

diff --git a/tools/binman/etype/blob.py b/tools/binman/etype/blob.py index ec94568ac0a..a4ff0efcebc 100644 --- a/tools/binman/etype/blob.py +++ b/tools/binman/etype/blob.py @@ -41,17 +41,26 @@ class Entry_blob(Entry): self.ReadBlobContents() return True

- def ReadBlobContents(self): - # We assume the data is small enough to fit into memory. If this - # is used for large filesystem image that might not be true. - # In that case, Image.BuildImage() could be adjusted to use a - # new Entry method which can read in chunks. Then we could copy - # the data in chunks and avoid reading it all at once. For now - # this seems like an unnecessary complication. - indata = tools.ReadFile(self._pathname) + def CompressData(self, indata): if self.compress != 'none': self.uncomp_size = len(indata) data = tools.Compress(indata, self.compress) + return data + + def ReadBlobContents(self): + """Read blob contents into memory + + This function compresses the data before storing if needed. + + We assume the data is small enough to fit into memory. If this + is used for large filesystem image that might not be true. + In that case, Image.BuildImage() could be adjusted to use a + new Entry method which can read in chunks. Then we could copy + the data in chunks and avoid reading it all at once. For now + this seems like an unnecessary complication. + """ + indata = tools.ReadFile(self._pathname) + data = self.CompressData(indata) self.SetContents(data) return True

diff --git a/tools/binman/etype/blob_dtb.py b/tools/binman/etype/blob_dtb.py index 09d5d727138..88ed55d8865 100644 --- a/tools/binman/etype/blob_dtb.py +++ b/tools/binman/etype/blob_dtb.py @@ -23,11 +23,11 @@ class Entry_blob_dtb(Entry_blob): def ObtainContents(self): """Get the device-tree from the list held by the 'state' module""" self._filename = self.GetDefaultFilename() - self._pathname, data = state.GetFdtContents(self._filename) - self.SetContents(data) - return True + self._pathname, _ = state.GetFdtContents(self._filename) + return Entry_blob.ReadBlobContents(self)

def ProcessContents(self): """Re-read the DTB contents so that we get any calculated properties""" - _, data = state.GetFdtContents(self._filename) + _, indata = state.GetFdtContents(self._filename) + data = self.CompressData(indata) return self.ProcessContentsUpdate(data) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index aae8dbc1b33..89e732fca31 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -2142,6 +2142,24 @@ class TestFunctional(unittest.TestCase): self.assertEqual(U_BOOT_DATA, data[2:2 + len(U_BOOT_DATA)]) self.assertEqual(b'aa', data[-2:])

+ def testCompressDtb(self): + """Test that compress of device-tree files is supported""" + self._CheckLz4() + data = self.data = self._DoReadFileRealDtb('124_compress_dtb.dts') + self.assertEqual(U_BOOT_DATA, data[:len(U_BOOT_DATA)]) + comp_data = data[len(U_BOOT_DATA):] + orig = self._decompress(comp_data) + dtb = fdt.Fdt.FromData(orig) + dtb.Scan() + props = self._GetPropTree(dtb, ['size', 'uncomp-size']) + expected = { + 'u-boot:size': len(U_BOOT_DATA), + 'u-boot-dtb:uncomp-size': len(orig), + 'u-boot-dtb:size': len(comp_data), + 'size': len(data), + } + self.assertEqual(expected, props) +

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/124_compress_dtb.dts b/tools/binman/test/124_compress_dtb.dts new file mode 100644 index 00000000000..46bfd8b265f --- /dev/null +++ b/tools/binman/test/124_compress_dtb.dts @@ -0,0 +1,14 @@ +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + u-boot { + }; + u-boot-dtb { + compress = "lz4"; + }; + }; +};

At present a file with no explicit CBFS offset is placed in the next available location but there is no way to find out where it ended up. Update and rename the get_data() function to provide this information.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/cbfs_util.py | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/tools/binman/cbfs_util.py b/tools/binman/cbfs_util.py index 1cdbcb2339e..530629a5c96 100644 --- a/tools/binman/cbfs_util.py +++ b/tools/binman/cbfs_util.py @@ -185,7 +185,8 @@ class CbfsFile(object): """Class to represent a single CBFS file

This is used to hold the information about a file, including its contents. - Use the get_data() method to obtain the raw output for writing to CBFS. + Use the get_data_and_offset() method to obtain the raw output for writing to + CBFS.

Properties: name: Name of file @@ -319,12 +320,15 @@ class CbfsFile(object): raise ValueError('Unknown file type %#x\n' % self.ftype) return hdr_len

- def get_data(self, offset=None, pad_byte=None): - """Obtain the contents of the file, in CBFS format + def get_data_and_offset(self, offset=None, pad_byte=None): + """Obtain the contents of the file, in CBFS format and the offset of + the data within the file

Returns: - bytes representing the contents of this file, packed and aligned - for directly inserting into the final CBFS output + tuple: + bytes representing the contents of this file, packed and aligned + for directly inserting into the final CBFS output + offset to the file data from the start of the returned data. """ name = _pack_string(self.name) hdr_len = len(name) + FILE_HEADER_LEN @@ -368,8 +372,10 @@ class CbfsFile(object): (self.name, self.cbfs_offset, offset)) pad = tools.GetBytes(pad_byte, pad_len) hdr_len += pad_len - self.offset = len(content) + len(data) - hdr = struct.pack(FILE_HEADER_FORMAT, FILE_MAGIC, self.offset, + + # This is the offset of the start of the file's data, + size = len(content) + len(data) + hdr = struct.pack(FILE_HEADER_FORMAT, FILE_MAGIC, size, self.ftype, attr_pos, hdr_len)

# Do a sanity check of the get_header_len() function, to ensure that it @@ -381,7 +387,7 @@ class CbfsFile(object): # happen. It probably indicates that get_header_len() is broken. raise ValueError("Internal error: CBFS file '%s': Expected headers of %#x bytes, got %#d" % (self.name, expected_len, actual_len)) - return hdr + name + attr + pad + content + data + return hdr + name + attr + pad + content + data, hdr_len

class CbfsWriter(object): @@ -392,7 +398,7 @@ class CbfsWriter(object): cbw = CbfsWriter(size) cbw.add_file_raw('u-boot', tools.ReadFile('u-boot.bin')) ... - data = cbw.get_data() + data, cbfs_offset = cbw.get_data_and_offset()

Attributes: _master_name: Name of the file containing the master header @@ -475,7 +481,7 @@ class CbfsWriter(object): todo = align_int_down(offset - upto, self._align) if todo: cbf = CbfsFile.empty(todo, self._erase_byte) - fd.write(cbf.get_data()) + fd.write(cbf.get_data_and_offset()[0]) self._skip_to(fd, offset)

def _align_to(self, fd, align): @@ -579,8 +585,11 @@ class CbfsWriter(object): offset = cbf.calc_start_offset() if offset is not None: self._pad_to(fd, align_int_down(offset, self._align)) - fd.write(cbf.get_data(fd.tell(), self._erase_byte)) + pos = fd.tell() + data, data_offset = cbf.get_data_and_offset(pos, self._erase_byte) + fd.write(data) self._align_to(fd, self._align) + cbf.calced_cbfs_offset = pos + data_offset if not self._hdr_at_start: self._write_header(fd, add_fileheader=self._add_fileheader)

The purpose of this badly named field is a bit ambiguous. Adjust the code to use it only to store the uncompressed length of a file, leaving it set to None if there is no compression used. This makes it easy to see if the value in this field is relevant / useful.

Also set data_len for compressed fields, since it should be the length of the compressed data, not the uncompressed data.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/cbfs_util.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/tools/binman/cbfs_util.py b/tools/binman/cbfs_util.py index 530629a5c96..4691be4aee2 100644 --- a/tools/binman/cbfs_util.py +++ b/tools/binman/cbfs_util.py @@ -197,7 +197,8 @@ class CbfsFile(object): data_len: Length of (possibly compressed) data in bytes ftype: File type (TYPE_...) compression: Compression type (COMPRESS_...) - memlen: Length of data in memory (typically the uncompressed length) + memlen: Length of data in memory, i.e. the uncompressed length, None if + no compression algortihm is selected load: Load address in memory if known, else None entry: Entry address in memory if known, else None. This is where execution starts after the file is loaded @@ -213,11 +214,11 @@ class CbfsFile(object): self.data = data self.ftype = ftype self.compress = compress - self.memlen = len(data) + self.memlen = None self.load = None self.entry = None self.base_address = None - self.data_len = 0 + self.data_len = len(data) self.erase_byte = None self.size = None

@@ -349,9 +350,11 @@ class CbfsFile(object): data = tools.Compress(orig_data, 'lz4') elif self.compress == COMPRESS_LZMA: data = tools.Compress(orig_data, 'lzma') + self.memlen = len(orig_data) + self.data_len = len(data) attr = struct.pack(ATTR_COMPRESSION_FORMAT, FILE_ATTR_TAG_COMPRESSION, ATTR_COMPRESSION_LEN, - self.compress, len(orig_data)) + self.compress, self.memlen) elif self.ftype == TYPE_EMPTY: data = tools.GetBytes(self.erase_byte, self.size) else:

It is useful to add the CBFS file information (offset, size, etc.) into the FDT so that the layout is complete. Add support for this.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/etype/cbfs.py | 49 +++++++++++++++++++++++++-- tools/binman/ftest.py | 25 ++++++++++++++ tools/binman/test/125_cbfs_update.dts | 21 ++++++++++++ 3 files changed, 92 insertions(+), 3 deletions(-) create mode 100644 tools/binman/test/125_cbfs_update.dts

diff --git a/tools/binman/etype/cbfs.py b/tools/binman/etype/cbfs.py index 49baa6a4f63..a46bb98a033 100644 --- a/tools/binman/etype/cbfs.py +++ b/tools/binman/etype/cbfs.py @@ -11,6 +11,7 @@ import cbfs_util from cbfs_util import CbfsWriter from entry import Entry import fdt_util +import state

class Entry_cbfs(Entry): """Entry containing a Coreboot Filesystem (CBFS) @@ -181,11 +182,17 @@ class Entry_cbfs(Entry): if not entry.ObtainContents(): return False data = entry.GetData() + cfile = None if entry._type == 'raw': - cbfs.add_file_raw(entry._cbfs_name, data, entry._cbfs_offset, - entry._cbfs_compress) + cfile = cbfs.add_file_raw(entry._cbfs_name, data, + entry._cbfs_offset, + entry._cbfs_compress) elif entry._type == 'stage': - cbfs.add_file_stage(entry._cbfs_name, data, entry._cbfs_offset) + cfile = cbfs.add_file_stage(entry._cbfs_name, data, + entry._cbfs_offset) + if cfile: + entry._cbfs_file = cfile + entry.size = cfile.data_len data = cbfs.get_data() self.SetContents(data) return True @@ -203,3 +210,39 @@ class Entry_cbfs(Entry): self.Raise("Invalid compression in '%s': '%s'" % (node.name, compress)) self._cbfs_entries[entry._cbfs_name] = entry + + def SetImagePos(self, image_pos): + """Override this function to set all the entry properties from CBFS + + We can only do this once image_pos is known + + Args: + image_pos: Position of this entry in the image + """ + Entry.SetImagePos(self, image_pos) + + # Now update the entries with info from the CBFS entries + for entry in self._cbfs_entries.values(): + cfile = entry._cbfs_file + entry.size = cfile.data_len + entry.offset = cfile.calced_cbfs_offset + entry.image_pos = self.image_pos + entry.offset + if entry._cbfs_compress: + entry.uncomp_size = cfile.memlen + + def AddMissingProperties(self): + Entry.AddMissingProperties(self) + for entry in self._cbfs_entries.values(): + entry.AddMissingProperties() + if entry._cbfs_compress: + state.AddZeroProp(entry._node, 'uncomp-size') + + def SetCalculatedProperties(self): + """Set the value of device-tree properties calculated by binman""" + Entry.SetCalculatedProperties(self) + for entry in self._cbfs_entries.values(): + state.SetInt(entry._node, 'offset', entry.offset) + state.SetInt(entry._node, 'size', entry.size) + state.SetInt(entry._node, 'image-pos', entry.image_pos) + if entry.uncomp_size is not None: + state.SetInt(entry._node, 'uncomp-size', entry.uncomp_size) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 89e732fca31..999d8884aca 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -2160,6 +2160,31 @@ class TestFunctional(unittest.TestCase): } self.assertEqual(expected, props)

+ def testCbfsUpdateFdt(self): + """Test that we can update the device tree with CBFS offset/size info""" + self._CheckLz4() + data, _, _, out_dtb_fname = self._DoReadFileDtb('125_cbfs_update.dts', + update_dtb=True) + dtb = fdt.Fdt(out_dtb_fname) + dtb.Scan() + props = self._GetPropTree(dtb, ['offset', 'size', 'image-pos', + 'uncomp-size']) + del props['cbfs/u-boot:size'] + self.assertEqual({ + 'offset': 0, + 'size': len(data), + 'image-pos': 0, + 'cbfs:offset': 0, + 'cbfs:size': len(data), + 'cbfs:image-pos': 0, + 'cbfs/u-boot:offset': 0x38, + 'cbfs/u-boot:uncomp-size': len(U_BOOT_DATA), + 'cbfs/u-boot:image-pos': 0x38, + 'cbfs/u-boot-dtb:offset': 0xb8, + 'cbfs/u-boot-dtb:size': len(U_BOOT_DATA), + 'cbfs/u-boot-dtb:image-pos': 0xb8, + }, props) +

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/125_cbfs_update.dts b/tools/binman/test/125_cbfs_update.dts new file mode 100644 index 00000000000..6d2e8a0b8ff --- /dev/null +++ b/tools/binman/test/125_cbfs_update.dts @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + cbfs { + size = <0x100>; + u-boot { + cbfs-type = "raw"; + cbfs-compress = "lz4"; + }; + u-boot-dtb { + cbfs-type = "raw"; + }; + }; + }; +};

Detect when an unknown or unsupported file type is specified and report an error.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/etype/cbfs.py | 3 +++ tools/binman/ftest.py | 6 ++++++ tools/binman/test/126_cbfs_bad_type.dts | 17 +++++++++++++++++ 3 files changed, 26 insertions(+) create mode 100644 tools/binman/test/126_cbfs_bad_type.dts

diff --git a/tools/binman/etype/cbfs.py b/tools/binman/etype/cbfs.py index a46bb98a033..175ecae1584 100644 --- a/tools/binman/etype/cbfs.py +++ b/tools/binman/etype/cbfs.py @@ -190,6 +190,9 @@ class Entry_cbfs(Entry): elif entry._type == 'stage': cfile = cbfs.add_file_stage(entry._cbfs_name, data, entry._cbfs_offset) + else: + entry.Raise("Unknown cbfs-type '%s' (use 'raw', 'stage')" % + entry._type) if cfile: entry._cbfs_file = cfile entry.size = cfile.data_len diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 999d8884aca..21bea6c9d13 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -2185,6 +2185,12 @@ class TestFunctional(unittest.TestCase): 'cbfs/u-boot-dtb:image-pos': 0xb8, }, props)

+ def testCbfsBadType(self): + """Test an image header with a no specified location is detected""" + with self.assertRaises(ValueError) as e: + self._DoReadFile('126_cbfs_bad_type.dts') + self.assertIn("Unknown cbfs-type 'badtype'", str(e.exception)) +

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/126_cbfs_bad_type.dts b/tools/binman/test/126_cbfs_bad_type.dts new file mode 100644 index 00000000000..2cd6fc6d52d --- /dev/null +++ b/tools/binman/test/126_cbfs_bad_type.dts @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + cbfs { + size = <0x100>; + u-boot { + cbfs-type = "badtype"; + }; + }; + }; +};

It is useful to be able to summarise all the entries in an image, e.g. to display this to this user. Add a new ListEntries() method to Entry, and set up a way to call it through the Image class.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: - Add the Entry object to EntryInfo as well - Change Image.ListEntries() to BuildEntryList() since signature differs

tools/binman/bsection.py | 9 ++++ tools/binman/entry.py | 36 ++++++++++++++++ tools/binman/etype/cbfs.py | 7 +++- tools/binman/etype/section.py | 4 ++ tools/binman/ftest.py | 76 ++++++++++++++++++++++++++++++++++ tools/binman/image.py | 10 +++++ tools/binman/test/127_list.dts | 33 +++++++++++++++ 7 files changed, 174 insertions(+), 1 deletion(-) create mode 100644 tools/binman/test/127_list.dts

diff --git a/tools/binman/bsection.py b/tools/binman/bsection.py index 9047e55a34a..082f424241c 100644 --- a/tools/binman/bsection.py +++ b/tools/binman/bsection.py @@ -10,6 +10,7 @@ from __future__ import print_function from collections import OrderedDict import sys

+from entry import Entry import fdt_util import re import state @@ -512,3 +513,11 @@ class Section(object): image size is dynamic and its sections have not yet been packed """ return self._image._size + + def ListEntries(self, entries, indent): + """Override this method to list all files in the section""" + Entry.AddEntryInfo(entries, indent, self._name, 'section', self._size, + self._image_pos, None, self._offset, + self._parent_section) + for entry in self._entries.values(): + entry.ListEntries(entries, indent + 1) diff --git a/tools/binman/entry.py b/tools/binman/entry.py index e38cb71c596..ee63d183532 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -33,6 +33,10 @@ our_path = os.path.dirname(os.path.realpath(__file__)) # device-tree properties. EntryArg = namedtuple('EntryArg', ['name', 'datatype'])

+# Information about an entry for use when displaying summaries +EntryInfo = namedtuple('EntryInfo', ['indent', 'name', 'etype', 'size', + 'image_pos', 'uncomp_size', 'offset', + 'entry'])

class Entry(object): """An Entry in the section @@ -617,3 +621,35 @@ features to produce new behaviours. if not self.HasSibling(name): return False return self.section.GetEntries()[name].image_pos + + @staticmethod + def AddEntryInfo(entries, indent, name, etype, size, image_pos, + uncomp_size, offset, entry): + """Add a new entry to the entries list + + Args: + entries: List (of EntryInfo objects) to add to + indent: Current indent level to add to list + name: Entry name (string) + etype: Entry type (string) + size: Entry size in bytes (int) + image_pos: Position within image in bytes (int) + uncomp_size: Uncompressed size if the entry uses compression, else + None + offset: Entry offset within parent in bytes (int) + entry: Entry object + """ + entries.append(EntryInfo(indent, name, etype, size, image_pos, + uncomp_size, offset, entry)) + + def ListEntries(self, entries, indent): + """Add files in this entry to the list of entries + + This can be overridden by subclasses which need different behaviour. + + Args: + entries: List (of EntryInfo objects) to add to + indent: Current indent level to add to list + """ + self.AddEntryInfo(entries, indent, self.name, self.etype, self.size, + self.image_pos, self.uncomp_size, self.offset, self) diff --git a/tools/binman/etype/cbfs.py b/tools/binman/etype/cbfs.py index 175ecae1584..953d6f4868d 100644 --- a/tools/binman/etype/cbfs.py +++ b/tools/binman/etype/cbfs.py @@ -195,7 +195,6 @@ class Entry_cbfs(Entry): entry._type) if cfile: entry._cbfs_file = cfile - entry.size = cfile.data_len data = cbfs.get_data() self.SetContents(data) return True @@ -249,3 +248,9 @@ class Entry_cbfs(Entry): state.SetInt(entry._node, 'image-pos', entry.image_pos) if entry.uncomp_size is not None: state.SetInt(entry._node, 'uncomp-size', entry.uncomp_size) + + def ListEntries(self, entries, indent): + """Override this method to list all files in the section""" + Entry.ListEntries(self, entries, indent) + for entry in self._cbfs_entries.values(): + entry.ListEntries(entries, indent + 1) diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 23bf22113d4..178e89352e5 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -111,3 +111,7 @@ class Entry_section(Entry): def ExpandToLimit(self, limit): super(Entry_section, self).ExpandToLimit(limit) self._section.ExpandSize(self.size) + + def ListEntries(self, entries, indent): + """List the files in the section""" + self._section.ListEntries(entries, indent) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 21bea6c9d13..de459b2b3b6 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -2191,6 +2191,82 @@ class TestFunctional(unittest.TestCase): self._DoReadFile('126_cbfs_bad_type.dts') self.assertIn("Unknown cbfs-type 'badtype'", str(e.exception))

+ def testList(self): + """Test listing the files in an image""" + self._CheckLz4() + data = self._DoReadFile('127_list.dts') + image = control.images['image'] + entries = image.BuildEntryList() + self.assertEqual(7, len(entries)) + + ent = entries[0] + self.assertEqual(0, ent.indent) + self.assertEqual('main-section', ent.name) + self.assertEqual('section', ent.etype) + self.assertEqual(len(data), ent.size) + self.assertEqual(0, ent.image_pos) + self.assertEqual(None, ent.uncomp_size) + self.assertEqual(None, ent.offset) + + ent = entries[1] + self.assertEqual(1, ent.indent) + self.assertEqual('u-boot', ent.name) + self.assertEqual('u-boot', ent.etype) + self.assertEqual(len(U_BOOT_DATA), ent.size) + self.assertEqual(0, ent.image_pos) + self.assertEqual(None, ent.uncomp_size) + self.assertEqual(0, ent.offset) + + ent = entries[2] + self.assertEqual(1, ent.indent) + self.assertEqual('section', ent.name) + self.assertEqual('section', ent.etype) + section_size = ent.size + self.assertEqual(0x100, ent.image_pos) + self.assertEqual(None, ent.uncomp_size) + self.assertEqual(len(U_BOOT_DATA), ent.offset) + + ent = entries[3] + self.assertEqual(2, ent.indent) + self.assertEqual('cbfs', ent.name) + self.assertEqual('cbfs', ent.etype) + self.assertEqual(0x400, ent.size) + self.assertEqual(0x100, ent.image_pos) + self.assertEqual(None, ent.uncomp_size) + self.assertEqual(0, ent.offset) + + ent = entries[4] + self.assertEqual(3, ent.indent) + self.assertEqual('u-boot', ent.name) + self.assertEqual('u-boot', ent.etype) + self.assertEqual(len(U_BOOT_DATA), ent.size) + self.assertEqual(0x138, ent.image_pos) + self.assertEqual(None, ent.uncomp_size) + self.assertEqual(0x38, ent.offset) + + ent = entries[5] + self.assertEqual(3, ent.indent) + self.assertEqual('u-boot-dtb', ent.name) + self.assertEqual('text', ent.etype) + self.assertGreater(len(COMPRESS_DATA), ent.size) + self.assertEqual(0x178, ent.image_pos) + self.assertEqual(len(COMPRESS_DATA), ent.uncomp_size) + self.assertEqual(0x78, ent.offset) + + ent = entries[6] + self.assertEqual(2, ent.indent) + self.assertEqual('u-boot-dtb', ent.name) + self.assertEqual('u-boot-dtb', ent.etype) + self.assertEqual(0x500, ent.image_pos) + self.assertEqual(len(U_BOOT_DTB_DATA), ent.uncomp_size) + dtb_size = ent.size + # Compressing this data expands it since headers are added + self.assertGreater(dtb_size, len(U_BOOT_DTB_DATA)) + self.assertEqual(0x400, ent.offset) + + self.assertEqual(len(data), 0x100 + section_size) + self.assertEqual(section_size, 0x400 + dtb_size) +

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/image.py b/tools/binman/image.py index 6339d020e76..6f4bd5d37b2 100644 --- a/tools/binman/image.py +++ b/tools/binman/image.py @@ -162,3 +162,13 @@ class Image: file=fd) self._section.WriteMap(fd, 0) return fname + + def BuildEntryList(self): + """List the files in an image + + Returns: + List of entry.EntryInfo objects describing all entries in the image + """ + entries = [] + self._section.ListEntries(entries, 0) + return entries diff --git a/tools/binman/test/127_list.dts b/tools/binman/test/127_list.dts new file mode 100644 index 00000000000..c1d6fce3f9e --- /dev/null +++ b/tools/binman/test/127_list.dts @@ -0,0 +1,33 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + u-boot { + }; + section { + align = <0x100>; + cbfs { + size = <0x400>; + u-boot { + cbfs-type = "raw"; + cbfs-offset = <0x38>; + }; + u-boot-dtb { + type = "text"; + text = "compress xxxxxxxxxxxxxxxxxxxxxx data"; + cbfs-type = "raw"; + cbfs-compress = "lzma"; + cbfs-offset = <0x78>; + }; + }; + u-boot-dtb { + compress = "lz4"; + }; + }; + }; +};

Add support for locating an image's Fdt map which is used to determine the contents and structure of the image.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: - Adjust LocateFdtmap() to return the position of the header - Update commit subject to more accurately describe this patch - Update test to use _DoReadFileRealDtb() helper

tools/binman/etype/fdtmap.py | 25 ++++++++++++++++-- tools/binman/ftest.py | 15 +++++++++++ tools/binman/test/128_decode_image.dts | 36 ++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 2 deletions(-) create mode 100644 tools/binman/test/128_decode_image.dts

diff --git a/tools/binman/etype/fdtmap.py b/tools/binman/etype/fdtmap.py index bfd7962be3a..08505264984 100644 --- a/tools/binman/etype/fdtmap.py +++ b/tools/binman/etype/fdtmap.py @@ -15,7 +15,26 @@ from fdt import Fdt import state import tools

-FDTMAP_MAGIC = b'_FDTMAP_' +FDTMAP_MAGIC = b'_FDTMAP_' +FDTMAP_HDR_LEN = 16 + +def LocateFdtmap(data): + """Search an image for an fdt map + + Args: + data: Data to search + + Returns: + Position of fdt map in data, or None if not found. Note that the + position returned is of the FDT header, i.e. before the FDT data + """ + hdr_pos = data.find(FDTMAP_MAGIC) + size = len(data) + if hdr_pos: + hdr = data[hdr_pos:hdr_pos + FDTMAP_HDR_LEN] + if len(hdr) == FDTMAP_HDR_LEN: + return hdr_pos + return None

class Entry_fdtmap(Entry): """An entry which contains an FDT map @@ -24,7 +43,9 @@ class Entry_fdtmap(Entry): None

An FDT map is just a header followed by an FDT containing a list of all the - entries in the image. + entries in the image. The root node corresponds to the image node in the + original FDT, and an image-name property indicates the image name in that + original tree.

The header is the string _FDTMAP_ followed by 8 unused bytes.

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index de459b2b3b6..d800ba1e9d8 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -24,6 +24,7 @@ import command import control import elf import fdt +from etype import fdtmap import fdt_util import fmap_util import test_util @@ -2267,6 +2268,20 @@ class TestFunctional(unittest.TestCase): self.assertEqual(len(data), 0x100 + section_size) self.assertEqual(section_size, 0x400 + dtb_size)

+ def testFindFdtmap(self): + """Test locating an FDT map in an image""" + self._CheckLz4() + data = self.data = self._DoReadFileRealDtb('128_decode_image.dts') + image = control.images['image'] + entries = image.GetEntries() + entry = entries['fdtmap'] + self.assertEqual(entry.image_pos, fdtmap.LocateFdtmap(data)) + + def testFindFdtmapMissing(self): + """Test failing to locate an FDP map""" + data = self._DoReadFile('005_simple.dts') + self.assertEqual(None, fdtmap.LocateFdtmap(data)) +

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/128_decode_image.dts b/tools/binman/test/128_decode_image.dts new file mode 100644 index 00000000000..449fccc41df --- /dev/null +++ b/tools/binman/test/128_decode_image.dts @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + size = <0xc00>; + u-boot { + }; + section { + align = <0x100>; + cbfs { + size = <0x400>; + u-boot { + cbfs-type = "raw"; + }; + u-boot-dtb { + cbfs-type = "raw"; + cbfs-compress = "lzma"; + cbfs-offset = <0x80>; + }; + }; + u-boot-dtb { + compress = "lz4"; + }; + }; + fdtmap { + }; + image-header { + location = "end"; + }; + }; +};

Add support for locating an image header in an image.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/etype/image_header.py | 23 +++++++++++++++++++++++ tools/binman/ftest.py | 27 +++++++++++++++++++++++++++ 2 files changed, 50 insertions(+)

diff --git a/tools/binman/etype/image_header.py b/tools/binman/etype/image_header.py index b1c4f8a07e9..8f9c5aa5d9e 100644 --- a/tools/binman/etype/image_header.py +++ b/tools/binman/etype/image_header.py @@ -15,6 +15,29 @@ from entry import Entry import fdt_util

IMAGE_HEADER_MAGIC = b'BinM' +IMAGE_HEADER_LEN = 8 + +def LocateHeaderOffset(data): + """Search an image for an image header + + Args: + data: Data to search + + Returns: + Offset of image header in the image, or None if not found + """ + hdr_pos = data.find(IMAGE_HEADER_MAGIC) + if hdr_pos != -1: + size = len(data) + hdr = data[hdr_pos:hdr_pos + IMAGE_HEADER_LEN] + if len(hdr) == IMAGE_HEADER_LEN: + offset = struct.unpack('<I', hdr[4:])[0] + if hdr_pos == len(data) - IMAGE_HEADER_LEN: + pos = size + offset - (1 << 32) + else: + pos = offset + return pos + return None

class Entry_image_header(Entry): """An entry which contains a pointer to the FDT map diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index d800ba1e9d8..ce66e3a2f20 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -25,6 +25,7 @@ import control import elf import fdt from etype import fdtmap +from etype import image_header import fdt_util import fmap_util import test_util @@ -2282,6 +2283,32 @@ class TestFunctional(unittest.TestCase): data = self._DoReadFile('005_simple.dts') self.assertEqual(None, fdtmap.LocateFdtmap(data))

+ def testFindImageHeader(self): + """Test locating a image header""" + self._CheckLz4() + data = self._DoReadFileDtb('128_decode_image.dts', use_real_dtb=True, + update_dtb=True)[0] + image = control.images['image'] + entries = image.GetEntries() + entry = entries['fdtmap'] + # The header should point to the FDT map + self.assertEqual(entry.image_pos, image_header.LocateHeaderOffset(data)) + + def testFindImageHeaderStart(self): + """Test locating a image header located at the start of an image""" + data = self._DoReadFileDtb('117_fdtmap_hdr_start.dts', + use_real_dtb=True, update_dtb=True)[0] + image = control.images['image'] + entries = image.GetEntries() + entry = entries['fdtmap'] + # The header should point to the FDT map + self.assertEqual(entry.image_pos, image_header.LocateHeaderOffset(data)) + + def testFindImageHeaderMissing(self): + """Test failing to locate an image header""" + data = self._DoReadFile('005_simple.dts') + self.assertEqual(None, image_header.LocateHeaderOffset(data)) +

if __name__ == "__main__": unittest.main()

It is possible to read an Image, locate its FDT map and then read it into the binman data structures. This allows full access to the entries that were written to the image. Add support for this.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: - Deal with travis's old lz4 version by skipping tests as necessary - Move 129_list_fdtmap.dts into the next commit - Rename 128_decode_image_no_hdr.dts to 129_decode_image_nohdr.dts - Update Image for LocateFdtmap() returning the position of the header - Update test to use _DoReadFileRealDtb() helper

tools/binman/entry.py | 5 +++ tools/binman/ftest.py | 39 ++++++++++++++++++-- tools/binman/image.py | 38 +++++++++++++++++++ tools/binman/test/129_decode_image_nohdr.dts | 33 +++++++++++++++++ 4 files changed, 111 insertions(+), 4 deletions(-) create mode 100644 tools/binman/test/129_decode_image_nohdr.dts

diff --git a/tools/binman/entry.py b/tools/binman/entry.py index ee63d183532..6c74f2a2175 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -162,6 +162,11 @@ class Entry(object): self.orig_offset = self.offset self.orig_size = self.size

+ # These should not be set in input files, but are set in an FDT map, + # which is also read by this code. + self.image_pos = fdt_util.GetInt(self._node, 'image-pos') + self.uncomp_size = fdt_util.GetInt(self._node, 'uncomp-size') + self.align = fdt_util.GetInt(self._node, 'align') if tools.NotPowerOfTwo(self.align): raise ValueError("Node '%s': Alignment %s must be a power of two" % diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index ce66e3a2f20..f3a8e64ad13 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -30,6 +30,7 @@ import fdt_util import fmap_util import test_util import gzip +from image import Image import state import tools import tout @@ -2286,8 +2287,7 @@ class TestFunctional(unittest.TestCase): def testFindImageHeader(self): """Test locating a image header""" self._CheckLz4() - data = self._DoReadFileDtb('128_decode_image.dts', use_real_dtb=True, - update_dtb=True)[0] + data = self.data = self._DoReadFileRealDtb('128_decode_image.dts') image = control.images['image'] entries = image.GetEntries() entry = entries['fdtmap'] @@ -2296,8 +2296,7 @@ class TestFunctional(unittest.TestCase):

def testFindImageHeaderStart(self): """Test locating a image header located at the start of an image""" - data = self._DoReadFileDtb('117_fdtmap_hdr_start.dts', - use_real_dtb=True, update_dtb=True)[0] + data = self.data = self._DoReadFileRealDtb('117_fdtmap_hdr_start.dts') image = control.images['image'] entries = image.GetEntries() entry = entries['fdtmap'] @@ -2309,6 +2308,38 @@ class TestFunctional(unittest.TestCase): data = self._DoReadFile('005_simple.dts') self.assertEqual(None, image_header.LocateHeaderOffset(data))

+ def testReadImage(self): + """Test reading an image and accessing its FDT map""" + self._CheckLz4() + data = self.data = self._DoReadFileRealDtb('128_decode_image.dts') + image_fname = tools.GetOutputFilename('image.bin') + orig_image = control.images['image'] + image = Image.FromFile(image_fname) + self.assertEqual(orig_image.GetEntries().keys(), + image.GetEntries().keys()) + + orig_entry = orig_image.GetEntries()['fdtmap'] + entry = image.GetEntries()['fdtmap'] + self.assertEquals(orig_entry.offset, entry.offset) + self.assertEquals(orig_entry.size, entry.size) + self.assertEquals(orig_entry.image_pos, entry.image_pos) + + def testReadImageNoHeader(self): + """Test accessing an image's FDT map without an image header""" + self._CheckLz4() + data = self._DoReadFileRealDtb('129_decode_image_nohdr.dts') + image_fname = tools.GetOutputFilename('image.bin') + image = Image.FromFile(image_fname) + self.assertTrue(isinstance(image, Image)) + self.assertEqual('image', image._name) + + def testReadImageFail(self): + """Test failing to read an image image's FDT map""" + self._DoReadFile('005_simple.dts') + image_fname = tools.GetOutputFilename('image.bin') + with self.assertRaises(ValueError) as e: + image = Image.FromFile(image_fname) + self.assertIn("Cannot find FDT map in image", str(e.exception))

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/image.py b/tools/binman/image.py index 6f4bd5d37b2..f890350a8d0 100644 --- a/tools/binman/image.py +++ b/tools/binman/image.py @@ -12,6 +12,9 @@ from operator import attrgetter import re import sys

+from etype import fdtmap +from etype import image_header +import fdt import fdt_util import bsection import tools @@ -47,6 +50,41 @@ class Image: else: self._ReadNode()

+ @classmethod + def FromFile(cls, fname): + """Convert an image file into an Image for use in binman + + Args: + fname: Filename of image file to read + + Returns: + Image object on success + + Raises: + ValueError if something goes wrong + """ + data = tools.ReadFile(fname) + size = len(data) + + # First look for an image header + pos = image_header.LocateHeaderOffset(data) + if pos is None: + # Look for the FDT map + pos = fdtmap.LocateFdtmap(data) + if pos is None: + raise ValueError('Cannot find FDT map in image') + + # We don't know the FDT size, so check its header first + probe_dtb = fdt.Fdt.FromData( + data[pos + fdtmap.FDTMAP_HDR_LEN:pos + 256]) + dtb_size = probe_dtb.GetFdtObj().totalsize() + fdtmap_data = data[pos:pos + dtb_size + fdtmap.FDTMAP_HDR_LEN] + dtb = fdt.Fdt.FromData(fdtmap_data[fdtmap.FDTMAP_HDR_LEN:]) + dtb.Scan() + + # Return an Image with the associated nodes + return Image('image', dtb.GetRoot()) + def _ReadNode(self): """Read properties from the image node""" self._size = fdt_util.GetInt(self._node, 'size') diff --git a/tools/binman/test/129_decode_image_nohdr.dts b/tools/binman/test/129_decode_image_nohdr.dts new file mode 100644 index 00000000000..90fdd8820ca --- /dev/null +++ b/tools/binman/test/129_decode_image_nohdr.dts @@ -0,0 +1,33 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + size = <0xc00>; + u-boot { + }; + section { + align = <0x100>; + cbfs { + size = <0x400>; + u-boot { + cbfs-type = "raw"; + }; + u-boot-dtb { + cbfs-type = "raw"; + cbfs-compress = "lzma"; + cbfs-offset = <0x80>; + }; + }; + u-boot-dtb { + compress = "lz4"; + }; + }; + fdtmap { + }; + }; +};

When support for sections (and thus hierarchical images) was added to binman, the decision was made to create a new Section class which could be used by both Image and an Entry_section class. The decision between using inheritance and composition was tricky to make, but in the end it was decided that Image was different enough from Entry that it made sense to put the implementation of sections in an entirely separate class. It also has the advantage that core Image code does have to rely on an entry class in the etype directory.

This work was mostly completed in commit:

8f1da50ccc "binman: Refactor much of the image code into 'section'

As a result of this, the Section class has its own version of things like offset and size and these must be kept in sync with the parent Entry_section class in some cases.

In the last year it has become apparent that the cost of keeping things in sync is larger than expected, since more and more code wants to access these properties.

An alternative approach, previously considered and rejected, now seems better.

Adjust Image to be a subclass of Entry_section. Move the code from Section (in bsection.py) to Entry_section and delete Section. Update all tests accordingly.

This requires substantial changes to Image. Overall the changes reduce code size by about 240 lines. While much of that is just boilerplate from Section, there are quite a few functions in Entry_section which now do not need to be overiden from Entry. This suggests the change is beneficial even without further functionality being added.

A side benefit is that the properties of sections are now consistent with other entries. This fixes a problem in testListCmd() where some properties are missing for sections.

Unfortunately this is a very large commit since it is not feasible to do the migration piecemeal. Given the substantial tests available and the 100% code coverage of binman, we should be able to do this safely.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/README.entries | 21 +- tools/binman/bsection.py | 523 ---------------------------------- tools/binman/entry.py | 8 +- tools/binman/etype/files.py | 3 +- tools/binman/etype/fmap.py | 2 +- tools/binman/etype/section.py | 431 +++++++++++++++++++++++++--- tools/binman/ftest.py | 29 +- tools/binman/image.py | 129 +++------ tools/binman/image_test.py | 18 +- 9 files changed, 462 insertions(+), 702 deletions(-) delete mode 100644 tools/binman/bsection.py

diff --git a/tools/binman/README.entries b/tools/binman/README.entries index 598d8278a70..7ce88ee5da8 100644 --- a/tools/binman/README.entries +++ b/tools/binman/README.entries @@ -521,16 +521,21 @@ Entry: section: Entry that contains other entries -------------------------------------------------

Properties / Entry arguments: (see binman README for more information) - - size: Size of section in bytes - - align-size: Align size to a particular power of two - - pad-before: Add padding before the entry - - pad-after: Add padding after the entry - - pad-byte: Pad byte to use when padding - - sort-by-offset: Reorder the entries by offset - - end-at-4gb: Used to build an x86 ROM which ends at 4GB (2^32) - - name-prefix: Adds a prefix to the name of every entry in the section + pad-byte: Pad byte to use when padding + sort-by-offset: True if entries should be sorted by offset, False if + they must be in-order in the device tree description + end-at-4gb: Used to build an x86 ROM which ends at 4GB (2^32) + skip-at-start: Number of bytes before the first entry starts. These + effectively adjust the starting offset of entries. For example, + if this is 16, then the first entry would start at 16. An entry + with offset = 20 would in fact be written at offset 4 in the image + file, since the first 16 bytes are skipped when writing. + name-prefix: Adds a prefix to the name of every entry in the section when writing out the map

+Since a section is also an entry, it inherits all the properies of entries +too. + A section is an entry which can contain other entries, thus allowing hierarchical images to be created. See 'Sections and hierarchical images' in the binman README for more information. diff --git a/tools/binman/bsection.py b/tools/binman/bsection.py deleted file mode 100644 index 082f424241c..00000000000 --- a/tools/binman/bsection.py +++ /dev/null @@ -1,523 +0,0 @@ -# SPDX-License-Identifier: GPL-2.0+ -# Copyright (c) 2018 Google, Inc -# Written by Simon Glass sjg@chromium.org -# -# Base class for sections (collections of entries) -# - -from __future__ import print_function - -from collections import OrderedDict -import sys - -from entry import Entry -import fdt_util -import re -import state -import tools - -class Section(object): - """A section which contains multiple entries - - A section represents a collection of entries. There must be one or more - sections in an image. Sections are used to group entries together. - - Attributes: - _node: Node object that contains the section definition in device tree - _parent_section: Parent Section object which created this Section - _size: Section size in bytes, or None if not known yet - _align_size: Section size alignment, or None - _pad_before: Number of bytes before the first entry starts. This - effectively changes the place where entry offset 0 starts - _pad_after: Number of bytes after the last entry ends. The last - entry will finish on or before this boundary - _pad_byte: Byte to use to pad the section where there is no entry - _sort: True if entries should be sorted by offset, False if they - must be in-order in the device tree description - _skip_at_start: Number of bytes before the first entry starts. These - effectively adjust the starting offset of entries. For example, - if _pad_before is 16, then the first entry would start at 16. - An entry with offset = 20 would in fact be written at offset 4 - in the image file. - _end_4gb: Indicates that the section ends at the 4GB boundary. This is - used for x86 images, which want to use offsets such that a memory - address (like 0xff800000) is the first entry offset. This causes - _skip_at_start to be set to the starting memory address. - _name_prefix: Prefix to add to the name of all entries within this - section - _entries: OrderedDict() of entries - _orig_offset: Original offset value read from node - _orig_size: Original size value read from node - """ - def __init__(self, name, parent_section, node, image, test=False): - global entry - global Entry - import entry - from entry import Entry - - self._parent_section = parent_section - self._name = name - self._node = node - self._image = image - self._offset = None - self._size = None - self._align_size = None - self._pad_before = 0 - self._pad_after = 0 - self._pad_byte = 0 - self._sort = False - self._skip_at_start = None - self._end_4gb = False - self._name_prefix = '' - self._entries = OrderedDict() - self._image_pos = None - if not test: - self._ReadNode() - self._ReadEntries() - - def _ReadNode(self): - """Read properties from the section node""" - self._offset = fdt_util.GetInt(self._node, 'offset') - self._size = fdt_util.GetInt(self._node, 'size') - self._orig_offset = self._offset - self._orig_size = self._size - self._align_size = fdt_util.GetInt(self._node, 'align-size') - if tools.NotPowerOfTwo(self._align_size): - self._Raise("Alignment size %s must be a power of two" % - self._align_size) - self._pad_before = fdt_util.GetInt(self._node, 'pad-before', 0) - self._pad_after = fdt_util.GetInt(self._node, 'pad-after', 0) - self._pad_byte = fdt_util.GetInt(self._node, 'pad-byte', 0) - self._sort = fdt_util.GetBool(self._node, 'sort-by-offset') - self._end_4gb = fdt_util.GetBool(self._node, 'end-at-4gb') - self._skip_at_start = fdt_util.GetInt(self._node, 'skip-at-start') - if self._end_4gb: - if not self._size: - self._Raise("Section size must be provided when using end-at-4gb") - if self._skip_at_start is not None: - self._Raise("Provide either 'end-at-4gb' or 'skip-at-start'") - else: - self._skip_at_start = 0x100000000 - self._size - else: - if self._skip_at_start is None: - self._skip_at_start = 0 - self._name_prefix = fdt_util.GetString(self._node, 'name-prefix') - - def _ReadEntries(self): - for node in self._node.subnodes: - if node.name == 'hash': - continue - entry = Entry.Create(self, node) - entry.SetPrefix(self._name_prefix) - self._entries[node.name] = entry - - def GetFdtSet(self): - """Get the set of device tree files used by this image""" - fdt_set = set() - for entry in self._entries.values(): - fdt_set.update(entry.GetFdtSet()) - return fdt_set - - def SetOffset(self, offset): - self._offset = offset - - def ExpandEntries(self): - for entry in self._entries.values(): - entry.ExpandEntries() - - def AddMissingProperties(self): - """Add new properties to the device tree as needed for this entry""" - for prop in ['offset', 'size', 'image-pos']: - if not prop in self._node.props: - state.AddZeroProp(self._node, prop) - state.CheckAddHashProp(self._node) - for entry in self._entries.values(): - entry.AddMissingProperties() - - def SetCalculatedProperties(self): - state.SetInt(self._node, 'offset', self._offset or 0) - state.SetInt(self._node, 'size', self._size) - image_pos = self._image_pos - if self._parent_section: - image_pos -= self._parent_section.GetRootSkipAtStart() - state.SetInt(self._node, 'image-pos', image_pos) - for entry in self._entries.values(): - entry.SetCalculatedProperties() - - def ProcessFdt(self, fdt): - todo = self._entries.values() - for passnum in range(3): - next_todo = [] - for entry in todo: - if not entry.ProcessFdt(fdt): - next_todo.append(entry) - todo = next_todo - if not todo: - break - if todo: - self._Raise('Internal error: Could not complete processing of Fdt: ' - 'remaining %s' % todo) - return True - - def CheckSize(self): - """Check that the section contents does not exceed its size, etc.""" - contents_size = 0 - for entry in self._entries.values(): - contents_size = max(contents_size, entry.offset + entry.size) - - contents_size -= self._skip_at_start - - size = self._size - if not size: - size = self._pad_before + contents_size + self._pad_after - size = tools.Align(size, self._align_size) - - if self._size and contents_size > self._size: - self._Raise("contents size %#x (%d) exceeds section size %#x (%d)" % - (contents_size, contents_size, self._size, self._size)) - if not self._size: - self._size = size - if self._size != tools.Align(self._size, self._align_size): - self._Raise("Size %#x (%d) does not match align-size %#x (%d)" % - (self._size, self._size, self._align_size, self._align_size)) - return size - - def _Raise(self, msg): - """Raises an error for this section - - Args: - msg: Error message to use in the raise string - Raises: - ValueError() - """ - raise ValueError("Section '%s': %s" % (self._node.path, msg)) - - def GetPath(self): - """Get the path of an image (in the FDT) - - Returns: - Full path of the node for this image - """ - return self._node.path - - def FindEntryType(self, etype): - """Find an entry type in the section - - Args: - etype: Entry type to find - Returns: - entry matching that type, or None if not found - """ - for entry in self._entries.values(): - if entry.etype == etype: - return entry - return None - - def GetEntryContents(self): - """Call ObtainContents() for each entry - - This calls each entry's ObtainContents() a few times until they all - return True. We stop calling an entry's function once it returns - True. This allows the contents of one entry to depend on another. - - After 3 rounds we give up since it's likely an error. - """ - todo = self._entries.values() - for passnum in range(3): - next_todo = [] - for entry in todo: - if not entry.ObtainContents(): - next_todo.append(entry) - todo = next_todo - if not todo: - break - if todo: - self._Raise('Internal error: Could not complete processing of ' - 'contents: remaining %s' % todo) - return True - - def _SetEntryOffsetSize(self, name, offset, size): - """Set the offset and size of an entry - - Args: - name: Entry name to update - offset: New offset, or None to leave alone - size: New size, or None to leave alone - """ - entry = self._entries.get(name) - if not entry: - self._Raise("Unable to set offset/size for unknown entry '%s'" % - name) - entry.SetOffsetSize(self._skip_at_start + offset if offset else None, - size) - - def GetEntryOffsets(self): - """Handle entries that want to set the offset/size of other entries - - This calls each entry's GetOffsets() method. If it returns a list - of entries to update, it updates them. - """ - for entry in self._entries.values(): - offset_dict = entry.GetOffsets() - for name, info in offset_dict.items(): - self._SetEntryOffsetSize(name, *info) - - def ResetForPack(self): - """Reset offset/size fields so that packing can be done again""" - self._offset = self._orig_offset - self._size = self._orig_size - for entry in self._entries.values(): - entry.ResetForPack() - - def PackEntries(self): - """Pack all entries into the section""" - offset = self._skip_at_start - for entry in self._entries.values(): - offset = entry.Pack(offset) - self._size = self.CheckSize() - - def _SortEntries(self): - """Sort entries by offset""" - entries = sorted(self._entries.values(), key=lambda entry: entry.offset) - self._entries.clear() - for entry in entries: - self._entries[entry._node.name] = entry - - def _ExpandEntries(self): - """Expand any entries that are permitted to""" - exp_entry = None - for entry in self._entries.values(): - if exp_entry: - exp_entry.ExpandToLimit(entry.offset) - exp_entry = None - if entry.expand_size: - exp_entry = entry - if exp_entry: - exp_entry.ExpandToLimit(self._size) - - def CheckEntries(self): - """Check that entries do not overlap or extend outside the section - - This also sorts entries, if needed and expands - """ - if self._sort: - self._SortEntries() - self._ExpandEntries() - offset = 0 - prev_name = 'None' - for entry in self._entries.values(): - entry.CheckOffset() - if (entry.offset < self._skip_at_start or - entry.offset + entry.size > self._skip_at_start + self._size): - entry.Raise("Offset %#x (%d) is outside the section starting " - "at %#x (%d)" % - (entry.offset, entry.offset, self._skip_at_start, - self._skip_at_start)) - if entry.offset < offset: - entry.Raise("Offset %#x (%d) overlaps with previous entry '%s' " - "ending at %#x (%d)" % - (entry.offset, entry.offset, prev_name, offset, offset)) - offset = entry.offset + entry.size - prev_name = entry.GetPath() - - def SetImagePos(self, image_pos): - self._image_pos = image_pos - for entry in self._entries.values(): - entry.SetImagePos(image_pos) - - def ProcessEntryContents(self): - """Call the ProcessContents() method for each entry - - This is intended to adjust the contents as needed by the entry type. - - Returns: - True if no entries needed to change their size - """ - sizes_ok = True - for entry in self._entries.values(): - if not entry.ProcessContents(): - sizes_ok = False - print("Entry '%s' size change" % self._node.path) - return sizes_ok - - def WriteSymbols(self): - """Write symbol values into binary files for access at run time""" - for entry in self._entries.values(): - entry.WriteSymbols(self) - - def BuildSection(self, fd, base_offset): - """Write the section to a file""" - fd.seek(base_offset) - fd.write(self.GetData()) - - def GetData(self): - """Get the contents of the section""" - section_data = tools.GetBytes(self._pad_byte, self._size) - - for entry in self._entries.values(): - data = entry.GetData() - base = self._pad_before + entry.offset - self._skip_at_start - section_data = (section_data[:base] + data + - section_data[base + len(data):]) - return section_data - - def LookupSymbol(self, sym_name, optional, msg): - """Look up a symbol in an ELF file - - Looks up a symbol in an ELF file. Only entry types which come from an - ELF image can be used by this function. - - At present the only entry property supported is offset. - - Args: - sym_name: Symbol name in the ELF file to look up in the format - _binman_<entry>_prop_<property> where <entry> is the name of - the entry and <property> is the property to find (e.g. - _binman_u_boot_prop_offset). As a special case, you can append - _any to <entry> to have it search for any matching entry. E.g. - _binman_u_boot_any_prop_offset will match entries called u-boot, - u-boot-img and u-boot-nodtb) - optional: True if the symbol is optional. If False this function - will raise if the symbol is not found - msg: Message to display if an error occurs - - Returns: - Value that should be assigned to that symbol, or None if it was - optional and not found - - Raises: - ValueError if the symbol is invalid or not found, or references a - property which is not supported - """ - m = re.match(r'^_binman_(\w+)_prop_(\w+)$', sym_name) - if not m: - raise ValueError("%s: Symbol '%s' has invalid format" % - (msg, sym_name)) - entry_name, prop_name = m.groups() - entry_name = entry_name.replace('_', '-') - entry = self._entries.get(entry_name) - if not entry: - if entry_name.endswith('-any'): - root = entry_name[:-4] - for name in self._entries: - if name.startswith(root): - rest = name[len(root):] - if rest in ['', '-img', '-nodtb']: - entry = self._entries[name] - if not entry: - err = ("%s: Entry '%s' not found in list (%s)" % - (msg, entry_name, ','.join(self._entries.keys()))) - if optional: - print('Warning: %s' % err, file=sys.stderr) - return None - raise ValueError(err) - if prop_name == 'offset': - return entry.offset - elif prop_name == 'image_pos': - return entry.image_pos - else: - raise ValueError("%s: No such property '%s'" % (msg, prop_name)) - - def GetEntries(self): - """Get the dict of entries in a section - - Returns: - OrderedDict of entries in a section - """ - return self._entries - - def GetSize(self): - """Get the size of a section in bytes - - This is only meaningful if the section has a pre-defined size, or the - entries within it have been packed, so that the size has been - calculated. - - Returns: - Entry size in bytes - """ - return self._size - - def WriteMap(self, fd, indent): - """Write a map of the section to a .map file - - Args: - fd: File to write the map to - """ - Entry.WriteMapLine(fd, indent, self._name, self._offset or 0, - self._size, self._image_pos) - for entry in self._entries.values(): - entry.WriteMap(fd, indent + 1) - - def GetContentsByPhandle(self, phandle, source_entry): - """Get the data contents of an entry specified by a phandle - - This uses a phandle to look up a node and and find the entry - associated with it. Then it returnst he contents of that entry. - - Args: - phandle: Phandle to look up (integer) - source_entry: Entry containing that phandle (used for error - reporting) - - Returns: - data from associated entry (as a string), or None if not found - """ - node = self._node.GetFdt().LookupPhandle(phandle) - if not node: - source_entry.Raise("Cannot find node for phandle %d" % phandle) - for entry in self._entries.values(): - if entry._node == node: - return entry.GetData() - source_entry.Raise("Cannot find entry for node '%s'" % node.name) - - def ExpandSize(self, size): - """Change the size of an entry - - Args: - size: New size for entry - """ - if size != self._size: - self._size = size - - def GetRootSkipAtStart(self): - """Get the skip-at-start value for the top-level section - - This is used to find out the starting offset for root section that - contains this section. If this is a top-level section then it returns - the skip-at-start offset for this section. - - This is used to get the absolute position of section within the image. - - Returns: - Integer skip-at-start value for the root section containing this - section - """ - if self._parent_section: - return self._parent_section.GetRootSkipAtStart() - return self._skip_at_start - - def GetStartOffset(self): - """Get the start offset for this section - - Returns: - The first available offset in this section (typically 0) - """ - return self._skip_at_start - - def GetImageSize(self): - """Get the size of the image containing this section - - Returns: - Image size as an integer number of bytes, which may be None if the - image size is dynamic and its sections have not yet been packed - """ - return self._image._size - - def ListEntries(self, entries, indent): - """Override this method to list all files in the section""" - Entry.AddEntryInfo(entries, indent, self._name, 'section', self._size, - self._image_pos, None, self._offset, - self._parent_section) - for entry in self._entries.values(): - entry.ListEntries(entries, indent + 1) diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 6c74f2a2175..c45a2fdb4b0 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -175,8 +175,8 @@ class Entry(object): self.pad_after = fdt_util.GetInt(self._node, 'pad-after', 0) self.align_size = fdt_util.GetInt(self._node, 'align-size') if tools.NotPowerOfTwo(self.align_size): - raise ValueError("Node '%s': Alignment size %s must be a power " - "of two" % (self._node.path, self.align_size)) + self.Raise("Alignment size %s must be a power of two" % + self.align_size) self.align_end = fdt_util.GetInt(self._node, 'align-end') self.offset_unset = fdt_util.GetBool(self._node, 'offset-unset') self.expand_size = fdt_util.GetBool(self._node, 'expand-size') @@ -216,8 +216,8 @@ class Entry(object): """Set the value of device-tree properties calculated by binman""" state.SetInt(self._node, 'offset', self.offset) state.SetInt(self._node, 'size', self.size) - state.SetInt(self._node, 'image-pos', - self.image_pos - self.section.GetRootSkipAtStart()) + base = self.section.GetRootSkipAtStart() if self.section else 0 + state.SetInt(self._node, 'image-pos', self.image_pos - base) if self.uncomp_size is not None: state.SetInt(self._node, 'uncomp-size', self.uncomp_size) state.CheckSetHashValue(self._node, self.GetData) diff --git a/tools/binman/etype/files.py b/tools/binman/etype/files.py index 99f2f2f67fe..0068b305f75 100644 --- a/tools/binman/etype/files.py +++ b/tools/binman/etype/files.py @@ -14,7 +14,6 @@ import fdt_util import state import tools

-import bsection

class Entry_files(Entry_section): """Entry containing a set of files @@ -54,4 +53,4 @@ class Entry_files(Entry_section): state.AddString(subnode, 'compress', self._compress)

# Read entries again, now that we have some - self._section._ReadEntries() + self._ReadEntries() diff --git a/tools/binman/etype/fmap.py b/tools/binman/etype/fmap.py index 3a809486098..f8d8d866f13 100644 --- a/tools/binman/etype/fmap.py +++ b/tools/binman/etype/fmap.py @@ -49,7 +49,7 @@ class Entry_fmap(Entry): areas.append(fmap_util.FmapArea(pos or 0, entry.size or 0, tools.FromUnicode(entry.name), 0))

- entries = self.section._image.GetEntries() + entries = self.section.image.GetEntries() areas = [] for entry in entries.values(): _AddEntries(areas, entry) diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 178e89352e5..6db3c7a6f03 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -1,59 +1,155 @@ # SPDX-License-Identifier: GPL-2.0+ # Copyright (c) 2018 Google, Inc # Written by Simon Glass sjg@chromium.org -# -# Entry-type module for sections, which are entries which can contain other -# entries. -# + +"""Entry-type module for sections (groups of entries) + +Sections are entries which can contain other entries. This allows hierarchical +images to be created. +""" + +from __future__ import print_function + +from collections import OrderedDict +import re +import sys

from entry import Entry import fdt_util import tools

-import bsection

class Entry_section(Entry): """Entry that contains other entries

Properties / Entry arguments: (see binman README for more information) - - size: Size of section in bytes - - align-size: Align size to a particular power of two - - pad-before: Add padding before the entry - - pad-after: Add padding after the entry - - pad-byte: Pad byte to use when padding - - sort-by-offset: Reorder the entries by offset - - end-at-4gb: Used to build an x86 ROM which ends at 4GB (2^32) - - name-prefix: Adds a prefix to the name of every entry in the section + pad-byte: Pad byte to use when padding + sort-by-offset: True if entries should be sorted by offset, False if + they must be in-order in the device tree description + end-at-4gb: Used to build an x86 ROM which ends at 4GB (2^32) + skip-at-start: Number of bytes before the first entry starts. These + effectively adjust the starting offset of entries. For example, + if this is 16, then the first entry would start at 16. An entry + with offset = 20 would in fact be written at offset 4 in the image + file, since the first 16 bytes are skipped when writing. + name-prefix: Adds a prefix to the name of every entry in the section when writing out the map

+ Since a section is also an entry, it inherits all the properies of entries + too. + A section is an entry which can contain other entries, thus allowing hierarchical images to be created. See 'Sections and hierarchical images' in the binman README for more information. """ - def __init__(self, section, etype, node): - Entry.__init__(self, section, etype, node) - self._section = bsection.Section(node.name, section, node, - section._image) + def __init__(self, section, etype, node, test=False): + if not test: + Entry.__init__(self, section, etype, node) + if section: + self.image = section.image + self._entries = OrderedDict() + self._pad_byte = 0 + self._sort = False + self._skip_at_start = None + self._end_4gb = False + if not test: + self._ReadNode() + self._ReadEntries() + + def _Raise(self, msg): + """Raises an error for this section + + Args: + msg: Error message to use in the raise string + Raises: + ValueError() + """ + raise ValueError("Section '%s': %s" % (self._node.path, msg)) + + def _ReadNode(self): + """Read properties from the image node""" + self._pad_byte = fdt_util.GetInt(self._node, 'pad-byte', 0) + self._sort = fdt_util.GetBool(self._node, 'sort-by-offset') + self._end_4gb = fdt_util.GetBool(self._node, 'end-at-4gb') + self._skip_at_start = fdt_util.GetInt(self._node, 'skip-at-start') + if self._end_4gb: + if not self.size: + self.Raise("Section size must be provided when using end-at-4gb") + if self._skip_at_start is not None: + self.Raise("Provide either 'end-at-4gb' or 'skip-at-start'") + else: + self._skip_at_start = 0x100000000 - self.size + else: + if self._skip_at_start is None: + self._skip_at_start = 0 + self._name_prefix = fdt_util.GetString(self._node, 'name-prefix') + filename = fdt_util.GetString(self._node, 'filename') + if filename: + self._filename = filename + + def _ReadEntries(self): + for node in self._node.subnodes: + if node.name == 'hash': + continue + entry = Entry.Create(self, node) + entry.SetPrefix(self._name_prefix) + self._entries[node.name] = entry

def GetFdtSet(self): - return self._section.GetFdtSet() + fdt_set = set() + for entry in self._entries.values(): + fdt_set.update(entry.GetFdtSet()) + return fdt_set

def ProcessFdt(self, fdt): - return self._section.ProcessFdt(fdt) + """Allow entries to adjust the device tree + + Some entries need to adjust the device tree for their purposes. This + may involve adding or deleting properties. + """ + todo = self._entries.values() + for passnum in range(3): + next_todo = [] + for entry in todo: + if not entry.ProcessFdt(fdt): + next_todo.append(entry) + todo = next_todo + if not todo: + break + if todo: + self.Raise('Internal error: Could not complete processing of Fdt: remaining %s' % + todo) + return True

def ExpandEntries(self): + """Expand out any entries which have calculated sub-entries + + Some entries are expanded out at runtime, e.g. 'files', which produces + a section containing a list of files. Process these entries so that + this information is added to the device tree. + """ Entry.ExpandEntries(self) - self._section.ExpandEntries() + for entry in self._entries.values(): + entry.ExpandEntries()

def AddMissingProperties(self): + """Add new properties to the device tree as needed for this entry""" Entry.AddMissingProperties(self) - self._section.AddMissingProperties() + for entry in self._entries.values(): + entry.AddMissingProperties()

def ObtainContents(self): - return self._section.GetEntryContents() + return self.GetEntryContents()

def GetData(self): - return self._section.GetData() + section_data = tools.GetBytes(self._pad_byte, self.size) + + for entry in self._entries.values(): + data = entry.GetData() + base = self.pad_before + entry.offset - self._skip_at_start + section_data = (section_data[:base] + data + + section_data[base + len(data):]) + return section_data

def GetOffsets(self): """Handle entries that want to set the offset/size of other entries @@ -61,41 +157,94 @@ class Entry_section(Entry): This calls each entry's GetOffsets() method. If it returns a list of entries to update, it updates them. """ - self._section.GetEntryOffsets() + self.GetEntryOffsets() return {}

def ResetForPack(self): """Reset offset/size fields so that packing can be done again""" - self._section.ResetForPack() Entry.ResetForPack(self) + for entry in self._entries.values(): + entry.ResetForPack()

def Pack(self, offset): """Pack all entries into the section""" - self._section.PackEntries() - if self._section._offset is None: - self._section.SetOffset(offset) - self.size = self._section.GetSize() - return super(Entry_section, self).Pack(offset) + self._PackEntries() + return Entry.Pack(self, offset)

- def SetImagePos(self, image_pos): - Entry.SetImagePos(self, image_pos) - self._section.SetImagePos(image_pos + self.offset) + def _PackEntries(self): + """Pack all entries into the image""" + offset = self._skip_at_start + for entry in self._entries.values(): + offset = entry.Pack(offset) + self.size = self.CheckSize() + + def _ExpandEntries(self): + """Expand any entries that are permitted to""" + exp_entry = None + for entry in self._entries.values(): + if exp_entry: + exp_entry.ExpandToLimit(entry.offset) + exp_entry = None + if entry.expand_size: + exp_entry = entry + if exp_entry: + exp_entry.ExpandToLimit(self.size) + + def _SortEntries(self): + """Sort entries by offset""" + entries = sorted(self._entries.values(), key=lambda entry: entry.offset) + self._entries.clear() + for entry in entries: + self._entries[entry._node.name] = entry + + def CheckEntries(self): + """Check that entries do not overlap or extend outside the image""" + if self._sort: + self._SortEntries() + self._ExpandEntries() + offset = 0 + prev_name = 'None' + for entry in self._entries.values(): + entry.CheckOffset() + if (entry.offset < self._skip_at_start or + entry.offset + entry.size > self._skip_at_start + + self.size): + entry.Raise("Offset %#x (%d) is outside the section starting " + "at %#x (%d)" % + (entry.offset, entry.offset, self._skip_at_start, + self._skip_at_start)) + if entry.offset < offset: + entry.Raise("Offset %#x (%d) overlaps with previous entry '%s' " + "ending at %#x (%d)" % + (entry.offset, entry.offset, prev_name, offset, offset)) + offset = entry.offset + entry.size + prev_name = entry.GetPath()

def WriteSymbols(self, section): """Write symbol values into binary files for access at run time""" - self._section.WriteSymbols() + for entry in self._entries.values(): + entry.WriteSymbols(self)

def SetCalculatedProperties(self): Entry.SetCalculatedProperties(self) - self._section.SetCalculatedProperties() + for entry in self._entries.values(): + entry.SetCalculatedProperties() + + def SetImagePos(self, image_pos): + Entry.SetImagePos(self, image_pos) + for entry in self._entries.values(): + entry.SetImagePos(image_pos + self.offset)

def ProcessContents(self): - sizes_ok = self._section.ProcessEntryContents() sizes_ok_base = super(Entry_section, self).ProcessContents() + sizes_ok = True + for entry in self._entries.values(): + if not entry.ProcessContents(): + sizes_ok = False return sizes_ok and sizes_ok_base

def CheckOffset(self): - self._section.CheckEntries() + self.CheckEntries()

def WriteMap(self, fd, indent): """Write a map of the section to a .map file @@ -103,15 +252,211 @@ class Entry_section(Entry): Args: fd: File to write the map to """ - self._section.WriteMap(fd, indent) + Entry.WriteMapLine(fd, indent, self.name, self.offset or 0, + self.size, self.image_pos) + for entry in self._entries.values(): + entry.WriteMap(fd, indent + 1)

def GetEntries(self): - return self._section.GetEntries() + return self._entries + + def GetContentsByPhandle(self, phandle, source_entry): + """Get the data contents of an entry specified by a phandle + + This uses a phandle to look up a node and and find the entry + associated with it. Then it returnst he contents of that entry. + + Args: + phandle: Phandle to look up (integer) + source_entry: Entry containing that phandle (used for error + reporting) + + Returns: + data from associated entry (as a string), or None if not found + """ + node = self._node.GetFdt().LookupPhandle(phandle) + if not node: + source_entry.Raise("Cannot find node for phandle %d" % phandle) + for entry in self._entries.values(): + if entry._node == node: + return entry.GetData() + source_entry.Raise("Cannot find entry for node '%s'" % node.name) + + def LookupSymbol(self, sym_name, optional, msg): + """Look up a symbol in an ELF file + + Looks up a symbol in an ELF file. Only entry types which come from an + ELF image can be used by this function. + + At present the only entry property supported is offset. + + Args: + sym_name: Symbol name in the ELF file to look up in the format + _binman_<entry>_prop_<property> where <entry> is the name of + the entry and <property> is the property to find (e.g. + _binman_u_boot_prop_offset). As a special case, you can append + _any to <entry> to have it search for any matching entry. E.g. + _binman_u_boot_any_prop_offset will match entries called u-boot, + u-boot-img and u-boot-nodtb) + optional: True if the symbol is optional. If False this function + will raise if the symbol is not found + msg: Message to display if an error occurs + + Returns: + Value that should be assigned to that symbol, or None if it was + optional and not found + + Raises: + ValueError if the symbol is invalid or not found, or references a + property which is not supported + """ + m = re.match(r'^_binman_(\w+)_prop_(\w+)$', sym_name) + if not m: + raise ValueError("%s: Symbol '%s' has invalid format" % + (msg, sym_name)) + entry_name, prop_name = m.groups() + entry_name = entry_name.replace('_', '-') + entry = self._entries.get(entry_name) + if not entry: + if entry_name.endswith('-any'): + root = entry_name[:-4] + for name in self._entries: + if name.startswith(root): + rest = name[len(root):] + if rest in ['', '-img', '-nodtb']: + entry = self._entries[name] + if not entry: + err = ("%s: Entry '%s' not found in list (%s)" % + (msg, entry_name, ','.join(self._entries.keys()))) + if optional: + print('Warning: %s' % err, file=sys.stderr) + return None + raise ValueError(err) + if prop_name == 'offset': + return entry.offset + elif prop_name == 'image_pos': + return entry.image_pos + else: + raise ValueError("%s: No such property '%s'" % (msg, prop_name)) + + def GetRootSkipAtStart(self): + """Get the skip-at-start value for the top-level section + + This is used to find out the starting offset for root section that + contains this section. If this is a top-level section then it returns + the skip-at-start offset for this section. + + This is used to get the absolute position of section within the image. + + Returns: + Integer skip-at-start value for the root section containing this + section + """ + if self.section: + return self.section.GetRootSkipAtStart() + return self._skip_at_start + + def GetStartOffset(self): + """Get the start offset for this section + + Returns: + The first available offset in this section (typically 0) + """ + return self._skip_at_start + + def GetImageSize(self): + """Get the size of the image containing this section + + Returns: + Image size as an integer number of bytes, which may be None if the + image size is dynamic and its sections have not yet been packed + """ + return self.image.size + + def FindEntryType(self, etype): + """Find an entry type in the section + + Args: + etype: Entry type to find + Returns: + entry matching that type, or None if not found + """ + for entry in self._entries.values(): + if entry.etype == etype: + return entry + return None + + def GetEntryContents(self): + """Call ObtainContents() for the section + """ + todo = self._entries.values() + for passnum in range(3): + next_todo = [] + for entry in todo: + if not entry.ObtainContents(): + next_todo.append(entry) + todo = next_todo + if not todo: + break + if todo: + self.Raise('Internal error: Could not complete processing of contents: remaining %s' % + todo) + return True + + def _SetEntryOffsetSize(self, name, offset, size): + """Set the offset and size of an entry + + Args: + name: Entry name to update + offset: New offset, or None to leave alone + size: New size, or None to leave alone + """ + entry = self._entries.get(name) + if not entry: + self._Raise("Unable to set offset/size for unknown entry '%s'" % + name) + entry.SetOffsetSize(self._skip_at_start + offset if offset else None, + size) + + def GetEntryOffsets(self): + """Handle entries that want to set the offset/size of other entries + + This calls each entry's GetOffsets() method. If it returns a list + of entries to update, it updates them. + """ + for entry in self._entries.values(): + offset_dict = entry.GetOffsets() + for name, info in offset_dict.items(): + self._SetEntryOffsetSize(name, *info) + + + def CheckSize(self): + """Check that the image contents does not exceed its size, etc.""" + contents_size = 0 + for entry in self._entries.values(): + contents_size = max(contents_size, entry.offset + entry.size) + + contents_size -= self._skip_at_start + + size = self.size + if not size: + size = self.pad_before + contents_size + self.pad_after + size = tools.Align(size, self.align_size)

- def ExpandToLimit(self, limit): - super(Entry_section, self).ExpandToLimit(limit) - self._section.ExpandSize(self.size) + if self.size and contents_size > self.size: + self._Raise("contents size %#x (%d) exceeds section size %#x (%d)" % + (contents_size, contents_size, self.size, self.size)) + if not self.size: + self.size = size + if self.size != tools.Align(self.size, self.align_size): + self._Raise("Size %#x (%d) does not match align-size %#x (%d)" % + (self.size, self.size, self.align_size, + self.align_size)) + return size

def ListEntries(self, entries, indent): """List the files in the section""" - self._section.ListEntries(entries, indent) + Entry.AddEntryInfo(entries, indent, self.name, 'section', self.size, + self.image_pos, None, self.offset, self) + for entry in self._entries.values(): + entry.ListEntries(entries, indent + 1) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index f3a8e64ad13..4f58ce3c859 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -595,7 +595,7 @@ class TestFunctional(unittest.TestCase): self.assertEqual(0, retcode)

image = control.images['image1'] - self.assertEqual(len(U_BOOT_DATA), image._size) + self.assertEqual(len(U_BOOT_DATA), image.size) fname = tools.GetOutputFilename('image1.bin') self.assertTrue(os.path.exists(fname)) with open(fname, 'rb') as fd: @@ -603,7 +603,7 @@ class TestFunctional(unittest.TestCase): self.assertEqual(U_BOOT_DATA, data)

image = control.images['image2'] - self.assertEqual(3 + len(U_BOOT_DATA) + 5, image._size) + self.assertEqual(3 + len(U_BOOT_DATA) + 5, image.size) fname = tools.GetOutputFilename('image2.bin') self.assertTrue(os.path.exists(fname)) with open(fname, 'rb') as fd: @@ -659,7 +659,7 @@ class TestFunctional(unittest.TestCase): self.assertEqual(61, entry.offset) self.assertEqual(len(U_BOOT_DATA), entry.size)

- self.assertEqual(65, image._size) + self.assertEqual(65, image.size)

def testPackExtra(self): """Test that extra packing feature works as expected""" @@ -703,7 +703,7 @@ class TestFunctional(unittest.TestCase): self.assertEqual(64, entry.size)

self.CheckNoGaps(entries) - self.assertEqual(128, image._size) + self.assertEqual(128, image.size)

def testPackAlignPowerOf2(self): """Test that invalid entry alignment is detected""" @@ -761,7 +761,7 @@ class TestFunctional(unittest.TestCase): self.assertEqual(0, retcode) self.assertIn('image', control.images) image = control.images['image'] - self.assertEqual(7, image._size) + self.assertEqual(7, image.size)

def testPackImageSizeAlign(self): """Test that image size alignemnt works as expected""" @@ -769,7 +769,7 @@ class TestFunctional(unittest.TestCase): self.assertEqual(0, retcode) self.assertIn('image', control.images) image = control.images['image'] - self.assertEqual(16, image._size) + self.assertEqual(16, image.size)

def testPackInvalidImageAlign(self): """Test that invalid image alignment is detected""" @@ -782,7 +782,7 @@ class TestFunctional(unittest.TestCase): """Test that invalid image alignment is detected""" with self.assertRaises(ValueError) as e: self._DoTestFile('020_pack_inv_image_align_power2.dts') - self.assertIn("Section '/binman': Alignment size 131 must be a power of " + self.assertIn("Image '/binman': Alignment size 131 must be a power of " "two", str(e.exception))

def testImagePadByte(self): @@ -833,7 +833,7 @@ class TestFunctional(unittest.TestCase): """Test that the end-at-4gb property requires a size property""" with self.assertRaises(ValueError) as e: self._DoTestFile('027_pack_4gb_no_size.dts') - self.assertIn("Section '/binman': Section size must be provided when " + self.assertIn("Image '/binman': Section size must be provided when " "using end-at-4gb", str(e.exception))

def test4gbAndSkipAtStartTogether(self): @@ -841,7 +841,7 @@ class TestFunctional(unittest.TestCase): together""" with self.assertRaises(ValueError) as e: self._DoTestFile('80_4gb_and_skip_at_start_together.dts') - self.assertIn("Section '/binman': Provide either 'end-at-4gb' or " + self.assertIn("Image '/binman': Provide either 'end-at-4gb' or " "'skip-at-start'", str(e.exception))

def testPackX86RomOutside(self): @@ -1217,7 +1217,7 @@ class TestFunctional(unittest.TestCase): """Test that obtaining the contents works as expected""" with self.assertRaises(ValueError) as e: self._DoReadFile('057_unknown_contents.dts', True) - self.assertIn("Section '/binman': Internal error: Could not complete " + self.assertIn("Image '/binman': Internal error: Could not complete " "processing of contents: remaining [<_testing.Entry__testing ", str(e.exception))

@@ -1657,7 +1657,7 @@ class TestFunctional(unittest.TestCase): image = control.images['image'] entries = image.GetEntries() files = entries['files'] - entries = files._section._entries + entries = files._entries

orig = b'' for i in range(1, 3): @@ -2209,7 +2209,7 @@ class TestFunctional(unittest.TestCase): self.assertEqual(len(data), ent.size) self.assertEqual(0, ent.image_pos) self.assertEqual(None, ent.uncomp_size) - self.assertEqual(None, ent.offset) + self.assertEqual(0, ent.offset)

ent = entries[1] self.assertEqual(1, ent.indent) @@ -2227,7 +2227,7 @@ class TestFunctional(unittest.TestCase): section_size = ent.size self.assertEqual(0x100, ent.image_pos) self.assertEqual(None, ent.uncomp_size) - self.assertEqual(len(U_BOOT_DATA), ent.offset) + self.assertEqual(0x100, ent.offset)

ent = entries[3] self.assertEqual(2, ent.indent) @@ -2331,7 +2331,7 @@ class TestFunctional(unittest.TestCase): image_fname = tools.GetOutputFilename('image.bin') image = Image.FromFile(image_fname) self.assertTrue(isinstance(image, Image)) - self.assertEqual('image', image._name) + self.assertEqual('image', image.image_name)

def testReadImageFail(self): """Test failing to read an image image's FDT map""" @@ -2341,5 +2341,6 @@ class TestFunctional(unittest.TestCase): image = Image.FromFile(image_fname) self.assertIn("Cannot find FDT map in image", str(e.exception))

+ if __name__ == "__main__": unittest.main() diff --git a/tools/binman/image.py b/tools/binman/image.py index f890350a8d0..487290b6c4e 100644 --- a/tools/binman/image.py +++ b/tools/binman/image.py @@ -12,14 +12,15 @@ from operator import attrgetter import re import sys

+from entry import Entry from etype import fdtmap from etype import image_header +from etype import section import fdt import fdt_util -import bsection import tools

-class Image: +class Image(section.Entry_section): """A Image, representing an output from binman

An image is comprised of a collection of entries each containing binary @@ -27,12 +28,8 @@ class Image:

This class implements the various operations needed for images.

- Atrtributes: - _node: Node object that contains the image definition in device tree - _name: Image name - _size: Image size in bytes, or None if not known yet - _filename: Output filename for image - _sections: Sections present in this image (may be one or more) + Attributes: + filename: Output filename for image

Args: test: True if this is being called from a test of Images. This this case @@ -40,15 +37,15 @@ class Image: we create a section manually. """ def __init__(self, name, node, test=False): - self._node = node - self._name = name - self._size = None - self._filename = '%s.bin' % self._name - if test: - self._section = bsection.Section('main-section', None, self._node, - self, True) - else: - self._ReadNode() + self.image = self + section.Entry_section.__init__(self, None, 'section', node, test) + self.name = 'main-section' + self.image_name = name + self._filename = '%s.bin' % self.image_name + if not test: + filename = fdt_util.GetString(self._node, 'filename') + if filename: + self._filename = filename

@classmethod def FromFile(cls, fname): @@ -85,84 +82,17 @@ class Image: # Return an Image with the associated nodes return Image('image', dtb.GetRoot())

- def _ReadNode(self): - """Read properties from the image node""" - self._size = fdt_util.GetInt(self._node, 'size') - filename = fdt_util.GetString(self._node, 'filename') - if filename: - self._filename = filename - self._section = bsection.Section('main-section', None, self._node, self) - def Raise(self, msg): """Convenience function to raise an error referencing an image""" raise ValueError("Image '%s': %s" % (self._node.path, msg))

- def GetFdtSet(self): - """Get the set of device tree files used by this image""" - return self._section.GetFdtSet() - - def ExpandEntries(self): - """Expand out any entries which have calculated sub-entries - - Some entries are expanded out at runtime, e.g. 'files', which produces - a section containing a list of files. Process these entries so that - this information is added to the device tree. - """ - self._section.ExpandEntries() - - def AddMissingProperties(self): - """Add properties that are not present in the device tree - - When binman has completed packing the entries the offset and size of - each entry are known. But before this the device tree may not specify - these. Add any missing properties, with a dummy value, so that the - size of the entry is correct. That way we can insert the correct values - later. - """ - self._section.AddMissingProperties() - - def ProcessFdt(self, fdt): - """Allow entries to adjust the device tree - - Some entries need to adjust the device tree for their purposes. This - may involve adding or deleting properties. - """ - return self._section.ProcessFdt(fdt) - - def GetEntryContents(self): - """Call ObtainContents() for the section - """ - self._section.GetEntryContents() - - def GetEntryOffsets(self): - """Handle entries that want to set the offset/size of other entries - - This calls each entry's GetOffsets() method. If it returns a list - of entries to update, it updates them. - """ - self._section.GetEntryOffsets() - - def ResetForPack(self): - """Reset offset/size fields so that packing can be done again""" - self._section.ResetForPack() - def PackEntries(self): """Pack all entries into the image""" - self._section.PackEntries() - - def CheckSize(self): - """Check that the image contents does not exceed its size, etc.""" - self._size = self._section.CheckSize() - - def CheckEntries(self): - """Check that entries do not overlap or extend outside the image""" - self._section.CheckEntries() - - def SetCalculatedProperties(self): - self._section.SetCalculatedProperties() + section.Entry_section.Pack(self, 0)

def SetImagePos(self): - self._section.SetImagePos(0) + # This first section in the image so it starts at 0 + section.Entry_section.SetImagePos(self, 0)

def ProcessEntryContents(self): """Call the ProcessContents() method for each entry @@ -172,20 +102,27 @@ class Image: Returns: True if the new data size is OK, False if expansion is needed """ - return self._section.ProcessEntryContents() + sizes_ok = True + for entry in self._entries.values(): + if not entry.ProcessContents(): + sizes_ok = False + print("Entry '%s' size change" % self._node.path) + return sizes_ok

def WriteSymbols(self): """Write symbol values into binary files for access at run time""" - self._section.WriteSymbols() + section.Entry_section.WriteSymbols(self, self) + + def BuildSection(self, fd, base_offset): + """Write the section to a file""" + fd.seek(base_offset) + fd.write(self.GetData())

def BuildImage(self): """Write the image to a file""" fname = tools.GetOutputFilename(self._filename) with open(fname, 'wb') as fd: - self._section.BuildSection(fd, 0) - - def GetEntries(self): - return self._section.GetEntries() + self.BuildSection(fd, 0)

def WriteMap(self): """Write a map of the image to a .map file @@ -193,12 +130,12 @@ class Image: Returns: Filename of map file written """ - filename = '%s.map' % self._name + filename = '%s.map' % self.image_name fname = tools.GetOutputFilename(filename) with open(fname, 'w') as fd: print('%8s %8s %8s %s' % ('ImagePos', 'Offset', 'Size', 'Name'), file=fd) - self._section.WriteMap(fd, 0) + section.Entry_section.WriteMap(self, fd, 0) return fname

def BuildEntryList(self): @@ -208,5 +145,5 @@ class Image: List of entry.EntryInfo objects describing all entries in the image """ entries = [] - self._section.ListEntries(entries, 0) + self.ListEntries(entries, 0) return entries diff --git a/tools/binman/image_test.py b/tools/binman/image_test.py index 3775e1afb07..4004f789b7c 100644 --- a/tools/binman/image_test.py +++ b/tools/binman/image_test.py @@ -12,28 +12,25 @@ from test_util import capture_sys_output class TestImage(unittest.TestCase): def testInvalidFormat(self): image = Image('name', 'node', test=True) - section = image._section with self.assertRaises(ValueError) as e: - section.LookupSymbol('_binman_something_prop_', False, 'msg') + image.LookupSymbol('_binman_something_prop_', False, 'msg') self.assertIn( "msg: Symbol '_binman_something_prop_' has invalid format", str(e.exception))

def testMissingSymbol(self): image = Image('name', 'node', test=True) - section = image._section - section._entries = {} + image._entries = {} with self.assertRaises(ValueError) as e: - section.LookupSymbol('_binman_type_prop_pname', False, 'msg') + image.LookupSymbol('_binman_type_prop_pname', False, 'msg') self.assertIn("msg: Entry 'type' not found in list ()", str(e.exception))

def testMissingSymbolOptional(self): image = Image('name', 'node', test=True) - section = image._section - section._entries = {} + image._entries = {} with capture_sys_output() as (stdout, stderr): - val = section.LookupSymbol('_binman_type_prop_pname', True, 'msg') + val = image.LookupSymbol('_binman_type_prop_pname', True, 'msg') self.assertEqual(val, None) self.assertEqual("Warning: msg: Entry 'type' not found in list ()\n", stderr.getvalue()) @@ -41,8 +38,7 @@ class TestImage(unittest.TestCase):

def testBadProperty(self): image = Image('name', 'node', test=True) - section = image._section - section._entries = {'u-boot': 1} + image._entries = {'u-boot': 1} with self.assertRaises(ValueError) as e: - section.LookupSymbol('_binman_u_boot_prop_bad', False, 'msg') + image.LookupSymbol('_binman_u_boot_prop_bad', False, 'msg') self.assertIn("msg: No such property 'bad", str(e.exception))

Add support for listing the entries in an image. This relies on the image having an FDT map.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: - Adjust the test to not rely on lz4 compression version - Allow listing subsets of the image - Change list command from 'list' to 'ls' - Deal with travis's old lz4 version by skipping tests as necessary - Move patch 'Add a comment about CBFS test structure' to earlier series - Move patch 'Allow cbfstool to be optional with tests' to earlier series - Update output from 'list' command - Update test to use _DoReadFileRealDtb() helper

tools/binman/README | 44 +++++++- tools/binman/cmdline.py | 6 ++ tools/binman/control.py | 35 ++++++ tools/binman/ftest.py | 82 ++++++++++++++ tools/binman/image.py | 150 ++++++++++++++++++++++++++ tools/binman/test/130_list_fdtmap.dts | 36 +++++++ 6 files changed, 352 insertions(+), 1 deletion(-) create mode 100644 tools/binman/test/130_list_fdtmap.dts

diff --git a/tools/binman/README b/tools/binman/README index 9860633792f..146e0fd470a 100644 --- a/tools/binman/README +++ b/tools/binman/README @@ -490,6 +490,49 @@ see README.entries. This is generated from the source code using: binman entry-docs >tools/binman/README.entries

+Listing images +-------------- + +It is possible to list the entries in an existing firmware image created by +binman, provided that there is an 'fdtmap' entry in the image. For example: + + $ binman ls -i image.bin + Name Image-pos Size Entry-type Offset Uncomp-size + ---------------------------------------------------------------------- + main-section c00 section 0 + u-boot 0 4 u-boot 0 + section 5fc section 4 + cbfs 100 400 cbfs 0 + u-boot 138 4 u-boot 38 + u-boot-dtb 180 108 u-boot-dtb 80 3b5 + u-boot-dtb 500 1ff u-boot-dtb 400 3b5 + fdtmap 6fc 381 fdtmap 6fc + image-header bf8 8 image-header bf8 + +This shows the hierarchy of the image, the position, size and type of each +entry, the offset of each entry within its parent and the uncompressed size if +the entry is compressed. + +It is also possible to list just some files in an image, e.g. + + $ binman ls -i image.bin section/cbfs + Name Image-pos Size Entry-type Offset Uncomp-size + -------------------------------------------------------------------- + cbfs 100 400 cbfs 0 + u-boot 138 4 u-boot 38 + u-boot-dtb 180 108 u-boot-dtb 80 3b5 + +or with wildcards: + + $ binman ls -i image.bin "*cb*" "*head*" + Name Image-pos Size Entry-type Offset Uncomp-size + ---------------------------------------------------------------------- + cbfs 100 400 cbfs 0 + u-boot 138 4 u-boot 38 + u-boot-dtb 180 108 u-boot-dtb 80 3b5 + image-header bf8 8 image-header bf8 + + Hashing Entries ---------------

@@ -825,7 +868,6 @@ Some ideas: - Add an option to decode an image into the constituent binaries - Support building an image for a board (-b) more completely, with a configurable build directory -- Support listing files in images - Support logging of binman's operations, with different levels of verbosity - Support updating binaries in an image (with no size change / repacking) - Support updating binaries in an image (with repacking) diff --git a/tools/binman/cmdline.py b/tools/binman/cmdline.py index a002105fc77..508232eabb5 100644 --- a/tools/binman/cmdline.py +++ b/tools/binman/cmdline.py @@ -65,6 +65,12 @@ controlled by a description in the board device tree.''' entry_parser = subparsers.add_parser('entry-docs', help='Write out entry documentation (see README.entries)')

+ list_parser = subparsers.add_parser('ls', help='List files in an image') + list_parser.add_argument('-i', '--image', type=str, required=True, + help='Image filename to list') + list_parser.add_argument('paths', type=str, nargs='*', + help='Paths within file to list (wildcard)') + test_parser = subparsers.add_parser('test', help='Run tests') test_parser.add_argument('-P', '--processes', type=int, help='set number of processes to use for running tests') diff --git a/tools/binman/control.py b/tools/binman/control.py index 35faf115062..813c8b1bf9e 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -67,6 +67,37 @@ def WriteEntryDocs(modules, test_missing=None): from entry import Entry Entry.WriteDocs(modules, test_missing)

+ +def ListEntries(image_fname, entry_paths): + """List the entries in an image + + This decodes the supplied image and displays a table of entries from that + image, preceded by a header. + + Args: + image_fname: Image filename to process + entry_paths: List of wildcarded paths (e.g. ['*dtb*', 'u-boot*', + 'section/u-boot']) + """ + image = Image.FromFile(image_fname) + + entries, lines, widths = image.GetListEntries(entry_paths) + + num_columns = len(widths) + for linenum, line in enumerate(lines): + if linenum == 1: + # Print header line + print('-' * (sum(widths) + num_columns * 2)) + out = '' + for i, item in enumerate(line): + width = -widths[i] + if item.startswith('>'): + width = -width + item = item[1:] + txt = '%*s ' % (width, item) + out += txt + print(out.rstrip()) + def Binman(args): """The main control code for binman

@@ -87,6 +118,10 @@ def Binman(args): command.Run(pager, fname) return 0

+ if args.cmd == 'ls': + ListEntries(args.image, args.paths) + return 0 + # Try to figure out which device tree contains our image description if args.dt: dtb_fname = args.dt diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 4f58ce3c859..ad828041f30 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -2341,6 +2341,88 @@ class TestFunctional(unittest.TestCase): image = Image.FromFile(image_fname) self.assertIn("Cannot find FDT map in image", str(e.exception))

+ def testListCmd(self): + """Test listing the files in an image using an Fdtmap""" + self._CheckLz4() + data = self._DoReadFileRealDtb('130_list_fdtmap.dts') + + # lz4 compression size differs depending on the version + image = control.images['image'] + entries = image.GetEntries() + section_size = entries['section'].size + fdt_size = entries['section'].GetEntries()['u-boot-dtb'].size + fdtmap_offset = entries['fdtmap'].offset + + image_fname = tools.GetOutputFilename('image.bin') + with test_util.capture_sys_output() as (stdout, stderr): + self._DoBinman('ls', '-i', image_fname) + lines = stdout.getvalue().splitlines() + expected = [ +'Name Image-pos Size Entry-type Offset Uncomp-size', +'----------------------------------------------------------------------', +'main-section 0 c00 section 0', +' u-boot 0 4 u-boot 0', +' section 100 %x section 100' % section_size, +' cbfs 100 400 cbfs 0', +' u-boot 138 4 u-boot 38', +' u-boot-dtb 180 10f u-boot-dtb 80 3c9', +' u-boot-dtb 500 %x u-boot-dtb 400 3c9' % fdt_size, +' fdtmap %x 395 fdtmap %x' % + (fdtmap_offset, fdtmap_offset), +' image-header bf8 8 image-header bf8', + ] + self.assertEqual(expected, lines) + + def testListCmdFail(self): + """Test failing to list an image""" + self._DoReadFile('005_simple.dts') + image_fname = tools.GetOutputFilename('image.bin') + with self.assertRaises(ValueError) as e: + self._DoBinman('ls', '-i', image_fname) + self.assertIn("Cannot find FDT map in image", str(e.exception)) + + def _RunListCmd(self, paths, expected): + """List out entries and check the result + + Args: + paths: List of paths to pass to the list command + expected: Expected list of filenames to be returned, in order + """ + self._CheckLz4() + self._DoReadFileRealDtb('130_list_fdtmap.dts') + image_fname = tools.GetOutputFilename('image.bin') + image = Image.FromFile(image_fname) + lines = image.GetListEntries(paths)[1] + files = [line[0].strip() for line in lines[1:]] + self.assertEqual(expected, files) + + def testListCmdSection(self): + """Test listing the files in a section""" + self._RunListCmd(['section'], + ['section', 'cbfs', 'u-boot', 'u-boot-dtb', 'u-boot-dtb']) + + def testListCmdFile(self): + """Test listing a particular file""" + self._RunListCmd(['*u-boot-dtb'], ['u-boot-dtb', 'u-boot-dtb']) + + def testListCmdWildcard(self): + """Test listing a wildcarded file""" + self._RunListCmd(['*boot*'], + ['u-boot', 'u-boot', 'u-boot-dtb', 'u-boot-dtb']) + + def testListCmdWildcardMulti(self): + """Test listing a wildcarded file""" + self._RunListCmd(['*cb*', '*head*'], + ['cbfs', 'u-boot', 'u-boot-dtb', 'image-header']) + + def testListCmdEmpty(self): + """Test listing a wildcarded file""" + self._RunListCmd(['nothing'], []) + + def testListCmdPath(self): + """Test listing the files in a sub-entry of a section""" + self._RunListCmd(['section/cbfs'], ['cbfs', 'u-boot', 'u-boot-dtb']) +

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/image.py b/tools/binman/image.py index 487290b6c4e..2c5668e2a96 100644 --- a/tools/binman/image.py +++ b/tools/binman/image.py @@ -8,6 +8,7 @@ from __future__ import print_function

from collections import OrderedDict +import fnmatch from operator import attrgetter import re import sys @@ -147,3 +148,152 @@ class Image(section.Entry_section): entries = [] self.ListEntries(entries, 0) return entries + + def FindEntryPath(self, entry_path): + """Find an entry at a given path in the image + + Args: + entry_path: Path to entry (e.g. /ro-section/u-boot') + + Returns: + Entry object corresponding to that past + + Raises: + ValueError if no entry found + """ + parts = entry_path.split('/') + entries = self.GetEntries() + parent = '/' + for part in parts: + entry = entries.get(part) + if not entry: + raise ValueError("Entry '%s' not found in '%s'" % + (part, parent)) + parent = entry.GetPath() + entries = entry.GetEntries() + return entry + + def ReadData(self, decomp=True): + return self._data + + def GetListEntries(self, entry_paths): + """List the entries in an image + + This decodes the supplied image and returns a list of entries from that + image, preceded by a header. + + Args: + entry_paths: List of paths to match (each can have wildcards). Only + entries whose names match one of these paths will be printed + + Returns: + String error message if something went wrong, otherwise + 3-Tuple: + List of EntryInfo objects + List of lines, each + List of text columns, each a string + List of widths of each column + """ + def _EntryToStrings(entry): + """Convert an entry to a list of strings, one for each column + + Args: + entry: EntryInfo object containing information to output + + Returns: + List of strings, one for each field in entry + """ + def _AppendHex(val): + """Append a hex value, or an empty string if val is None + + Args: + val: Integer value, or None if none + """ + args.append('' if val is None else '>%x' % val) + + args = [' ' * entry.indent + entry.name] + _AppendHex(entry.image_pos) + _AppendHex(entry.size) + args.append(entry.etype) + _AppendHex(entry.offset) + _AppendHex(entry.uncomp_size) + return args + + def _DoLine(lines, line): + """Add a line to the output list + + This adds a line (a list of columns) to the output list. It also updates + the widths[] array with the maximum width of each column + + Args: + lines: List of lines to add to + line: List of strings, one for each column + """ + for i, item in enumerate(line): + widths[i] = max(widths[i], len(item)) + lines.append(line) + + def _NameInPaths(fname, entry_paths): + """Check if a filename is in a list of wildcarded paths + + Args: + fname: Filename to check + entry_paths: List of wildcarded paths (e.g. ['*dtb*', 'u-boot*', + 'section/u-boot']) + + Returns: + True if any wildcard matches the filename (using Unix filename + pattern matching, not regular expressions) + False if not + """ + for path in entry_paths: + if fnmatch.fnmatch(fname, path): + return True + return False + + entries = self.BuildEntryList() + + # This is our list of lines. Each item in the list is a list of strings, one + # for each column + lines = [] + HEADER = ['Name', 'Image-pos', 'Size', 'Entry-type', 'Offset', + 'Uncomp-size'] + num_columns = len(HEADER) + + # This records the width of each column, calculated as the maximum width of + # all the strings in that column + widths = [0] * num_columns + _DoLine(lines, HEADER) + + # We won't print anything unless it has at least this indent. So at the + # start we will print nothing, unless a path matches (or there are no + # entry paths) + MAX_INDENT = 100 + min_indent = MAX_INDENT + path_stack = [] + path = '' + indent = 0 + selected_entries = [] + for entry in entries: + if entry.indent > indent: + path_stack.append(path) + elif entry.indent < indent: + path_stack.pop() + if path_stack: + path = path_stack[-1] + '/' + entry.name + indent = entry.indent + + # If there are entry paths to match and we are not looking at a + # sub-entry of a previously matched entry, we need to check the path + if entry_paths and indent <= min_indent: + if _NameInPaths(path[1:], entry_paths): + # Print this entry and all sub-entries (=higher indent) + min_indent = indent + else: + # Don't print this entry, nor any following entries until we get + # a path match + min_indent = MAX_INDENT + continue + _DoLine(lines, _EntryToStrings(entry)) + selected_entries.append(entry) + return selected_entries, lines, widths diff --git a/tools/binman/test/130_list_fdtmap.dts b/tools/binman/test/130_list_fdtmap.dts new file mode 100644 index 00000000000..449fccc41df --- /dev/null +++ b/tools/binman/test/130_list_fdtmap.dts @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + size = <0xc00>; + u-boot { + }; + section { + align = <0x100>; + cbfs { + size = <0x400>; + u-boot { + cbfs-type = "raw"; + }; + u-boot-dtb { + cbfs-type = "raw"; + cbfs-compress = "lzma"; + cbfs-offset = <0x80>; + }; + }; + u-boot-dtb { + compress = "lz4"; + }; + }; + fdtmap { + }; + image-header { + location = "end"; + }; + }; +};

Binman generally operates silently but in some cases it is useful to see what Binman is actually doing at each step. Enable some logging output with different logging levels selectable via the -v flag.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/README | 19 ++++++++++++++++++- tools/binman/entry.py | 3 ++- tools/binman/image.py | 3 ++- tools/patman/tout.py | 10 +++++++++- 4 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/tools/binman/README b/tools/binman/README index 146e0fd470a..1655a9d50df 100644 --- a/tools/binman/README +++ b/tools/binman/README @@ -533,6 +533,24 @@ or with wildcards: image-header bf8 8 image-header bf8

+Logging +------- + +Binman normally operates silently unless there is an error, in which case it +just displays the error. The -D/--debug option can be used to create a full +backtrace when errors occur. + +Internally binman logs some output while it is running. This can be displayed +by increasing the -v/--verbosity from the default of 1: + + 0: silent + 1: warnings only + 2: notices (important messages) + 3: info about major operations + 4: detailed information about each operation + 5: debug (all output) + + Hashing Entries ---------------

@@ -868,7 +886,6 @@ Some ideas: - Add an option to decode an image into the constituent binaries - Support building an image for a board (-b) more completely, with a configurable build directory -- Support logging of binman's operations, with different levels of verbosity - Support updating binaries in an image (with no size change / repacking) - Support updating binaries in an image (with repacking) - Support adding FITs to an image diff --git a/tools/binman/entry.py b/tools/binman/entry.py index c45a2fdb4b0..33d3f1e4d42 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -23,6 +23,7 @@ import sys import fdt_util import state import tools +import tout

modules = {}

@@ -272,7 +273,7 @@ class Entry(object): new_size = len(data) if state.AllowEntryExpansion(): if new_size > self.contents_size: - print("Entry '%s' size change from %#x to %#x" % ( + tout.Debug("Entry '%s' size change from %#x to %#x" % ( self._node.path, self.contents_size, new_size)) # self.data will indicate the new size needed size_ok = False diff --git a/tools/binman/image.py b/tools/binman/image.py index 2c5668e2a96..bbb5e23c3b2 100644 --- a/tools/binman/image.py +++ b/tools/binman/image.py @@ -20,6 +20,7 @@ from etype import section import fdt import fdt_util import tools +import tout

class Image(section.Entry_section): """A Image, representing an output from binman @@ -107,7 +108,7 @@ class Image(section.Entry_section): for entry in self._entries.values(): if not entry.ProcessContents(): sizes_ok = False - print("Entry '%s' size change" % self._node.path) + tout.Debug("Entry '%s' size change" % self._node.path) return sizes_ok

def WriteSymbols(self): diff --git a/tools/patman/tout.py b/tools/patman/tout.py index 4957c7ae1df..15acce28cb9 100644 --- a/tools/patman/tout.py +++ b/tools/patman/tout.py @@ -131,13 +131,21 @@ def Info(msg): """ _Output(3, msg)

+def Detail(msg): + """Display a detailed message + + Args: + msg; Message to display. + """ + _Output(4, msg) + def Debug(msg): """Display a debug message

Args: msg; Message to display. """ - _Output(4, msg) + _Output(5, msg)

def UserOutput(msg): """Display a message regardless of the current output level.

It is useful to be able to extract entry contents from an image to see what is inside. Add a simple function to read the contents of an entry, decompressing it by default.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/control.py | 20 ++++++++++++++++++ tools/binman/entry.py | 21 +++++++++++++++++++ tools/binman/etype/blob.py | 13 ++++++++++++ tools/binman/ftest.py | 42 ++++++++++++++++++++++++++++++++++++++ tools/binman/image.py | 4 +++- 5 files changed, 99 insertions(+), 1 deletion(-)

diff --git a/tools/binman/control.py b/tools/binman/control.py index 813c8b1bf9e..b244e7a0cd0 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -98,6 +98,26 @@ def ListEntries(image_fname, entry_paths): out += txt print(out.rstrip())

+ +def ReadEntry(image_fname, entry_path, decomp=True): + """Extract an entry from an image + + This extracts the data from a particular entry in an image + + Args: + image_fname: Image filename to process + entry_path: Path to entry to extract + decomp: True to return uncompressed data, if the data is compress + False to return the raw data + + Returns: + data extracted from the entry + """ + image = Image.FromFile(image_fname) + entry = image.FindEntryPath(entry_path) + return entry.ReadData(decomp) + + def Binman(args): """The main control code for binman

diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 33d3f1e4d42..1c382f3b852 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -659,3 +659,24 @@ features to produce new behaviours. """ self.AddEntryInfo(entries, indent, self.name, self.etype, self.size, self.image_pos, self.uncomp_size, self.offset, self) + + def ReadData(self, decomp=True): + """Read the data for an entry from the image + + This is used when the image has been read in and we want to extract the + data for a particular entry from that image. + + Args: + decomp: True to decompress any compressed data before returning it; + False to return the raw, uncompressed data + + Returns: + Entry data (bytes) + """ + # Use True here so that we get an uncompressed section to work from, + # although compressed sections are currently not supported + data = self.section.ReadData(True) + tout.Info('%s: Reading data from offset %#x-%#x, size %#x (avail %#x)' % + (self.GetPath(), self.offset, self.offset + self.size, + self.size, len(data))) + return data[self.offset:self.offset + self.size] diff --git a/tools/binman/etype/blob.py b/tools/binman/etype/blob.py index a4ff0efcebc..00cad33718c 100644 --- a/tools/binman/etype/blob.py +++ b/tools/binman/etype/blob.py @@ -9,6 +9,7 @@ from entry import Entry import fdt_util import state import tools +import tout

class Entry_blob(Entry): """Entry containing an arbitrary binary blob @@ -66,3 +67,15 @@ class Entry_blob(Entry):

def GetDefaultFilename(self): return self._filename + + def ReadData(self, decomp=True): + indata = Entry.ReadData(self, decomp) + if decomp: + data = tools.Decompress(indata, self.compress) + if self.uncomp_size: + tout.Info("%s: Decompressing data size %#x with algo '%s' to data size %#x" % + (self.GetPath(), len(indata), self.compress, + len(data))) + else: + data = indata + return data diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index ad828041f30..c11dd1b85ab 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -69,6 +69,8 @@ FILES_DATA = (b"sorry I'm late\nOh, don't bother apologising, I'm " + COMPRESS_DATA = b'compress xxxxxxxxxxxxxxxxxxxxxx data' REFCODE_DATA = b'refcode'

+EXTRACT_DTB_SIZE = 0x3c9 +

class TestFunctional(unittest.TestCase): """Functional tests for binman @@ -2423,6 +2425,46 @@ class TestFunctional(unittest.TestCase): """Test listing the files in a sub-entry of a section""" self._RunListCmd(['section/cbfs'], ['cbfs', 'u-boot', 'u-boot-dtb'])

+ def _RunExtractCmd(self, entry_name, decomp=True): + """Extract an entry from an image + + Args: + entry_name: Entry name to extract + decomp: True to decompress the data if compressed, False to leave + it in its raw uncompressed format + + Returns: + data from entry + """ + self._CheckLz4() + self._DoReadFileRealDtb('130_list_fdtmap.dts') + image_fname = tools.GetOutputFilename('image.bin') + return control.ReadEntry(image_fname, entry_name, decomp) + + def testExtractSimple(self): + """Test extracting a single file""" + data = self._RunExtractCmd('u-boot') + self.assertEqual(U_BOOT_DATA, data) + + def testExtractBadEntry(self): + """Test extracting a bad section path""" + with self.assertRaises(ValueError) as e: + self._RunExtractCmd('section/does-not-exist') + self.assertIn("Entry 'does-not-exist' not found in '/section'", + str(e.exception)) + + def testExtractMissingFile(self): + """Test extracting file that does not exist""" + with self.assertRaises(IOError) as e: + control.ReadEntry('missing-file', 'name') + + def testExtractBadFile(self): + """Test extracting an invalid file""" + fname = os.path.join(self._indir, 'badfile') + tools.WriteFile(fname, b'') + with self.assertRaises(ValueError) as e: + control.ReadEntry(fname, 'name') +

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/image.py b/tools/binman/image.py index bbb5e23c3b2..fb6e591ca60 100644 --- a/tools/binman/image.py +++ b/tools/binman/image.py @@ -82,7 +82,9 @@ class Image(section.Entry_section): dtb.Scan()

# Return an Image with the associated nodes - return Image('image', dtb.GetRoot()) + image = Image('image', dtb.GetRoot()) + image._data = data + return image

def Raise(self, msg): """Convenience function to raise an error referencing an image"""

CBFS is a bit like a section but with a custom format. Provide the list of entries and the compression type to binman so that it can extract the data from the CBFS, just like any other part of the image.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/cbfs_util.py | 14 ++++++++++++++ tools/binman/etype/cbfs.py | 7 +++++++ 2 files changed, 21 insertions(+)

diff --git a/tools/binman/cbfs_util.py b/tools/binman/cbfs_util.py index 4691be4aee2..45e16da0aaa 100644 --- a/tools/binman/cbfs_util.py +++ b/tools/binman/cbfs_util.py @@ -142,6 +142,20 @@ def find_compress(find_name): return compress return None

+def compress_name(compress): + """Look up the name of a compression algorithm + + Args: + compress: Compression algorithm number to find (COMPRESS_...) + + Returns: + Compression algorithm name (string) + + Raises: + KeyError if the algorithm number is invalid + """ + return COMPRESS_NAMES[compress] + def align_int(val, align): """Align a value up to the given alignment

diff --git a/tools/binman/etype/cbfs.py b/tools/binman/etype/cbfs.py index 953d6f4868d..edf2189fd26 100644 --- a/tools/binman/etype/cbfs.py +++ b/tools/binman/etype/cbfs.py @@ -238,6 +238,10 @@ class Entry_cbfs(Entry): entry.AddMissingProperties() if entry._cbfs_compress: state.AddZeroProp(entry._node, 'uncomp-size') + # Store the 'compress' property, since we don't look at + # 'cbfs-compress' in Entry.ReadData() + state.AddString(entry._node, 'compress', + cbfs_util.compress_name(entry._cbfs_compress))

def SetCalculatedProperties(self): """Set the value of device-tree properties calculated by binman""" @@ -254,3 +258,6 @@ class Entry_cbfs(Entry): Entry.ListEntries(self, entries, indent) for entry in self._cbfs_entries.values(): entry.ListEntries(entries, indent + 1) + + def GetEntries(self): + return self._cbfs_entries

It is useful to be able to extract all binaries from the image, or a subset of them. Add a new 'extract' command to handle this.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: None

tools/binman/README | 25 +++++- tools/binman/cmdline.py | 13 +++ tools/binman/control.py | 60 +++++++++++++ tools/binman/ftest.py | 189 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 286 insertions(+), 1 deletion(-)

diff --git a/tools/binman/README b/tools/binman/README index 1655a9d50df..756c6a0010e 100644 --- a/tools/binman/README +++ b/tools/binman/README @@ -533,6 +533,30 @@ or with wildcards: image-header bf8 8 image-header bf8

+Extracting files from images +---------------------------- + +You can extract files from an existing firmware image created by binman, +provided that there is an 'fdtmap' entry in the image. For example: + + $ binman extract -i image.bin section/cbfs/u-boot + +which will write the uncompressed contents of that entry to the file 'u-boot' in +the current directory. You can also extract to a particular file, in this case +u-boot.bin: + + $ binman extract -i image.bin section/cbfs/u-boot -f u-boot.bin + +It is possible to extract all files into a destination directory, which will +put files in subdirectories matching the entry hierarchy: + + $ binman extract -i image.bin -O outdir + +or just a selection: + + $ binman extract -i image.bin "*u-boot*" -O outdir + + Logging -------

@@ -883,7 +907,6 @@ Some ideas: - Use of-platdata to make the information available to code that is unable to use device tree (such as a very small SPL image) - Allow easy building of images by specifying just the board name -- Add an option to decode an image into the constituent binaries - Support building an image for a board (-b) more completely, with a configurable build directory - Support updating binaries in an image (with no size change / repacking) diff --git a/tools/binman/cmdline.py b/tools/binman/cmdline.py index 508232eabb5..a43aec649ed 100644 --- a/tools/binman/cmdline.py +++ b/tools/binman/cmdline.py @@ -71,6 +71,19 @@ controlled by a description in the board device tree.''' list_parser.add_argument('paths', type=str, nargs='*', help='Paths within file to list (wildcard)')

+ extract_parser = subparsers.add_parser('extract', + help='Extract files from an image') + extract_parser.add_argument('-i', '--image', type=str, required=True, + help='Image filename to extract') + extract_parser.add_argument('-f', '--filename', type=str, + help='Output filename to write to') + extract_parser.add_argument('-O', '--outdir', type=str, default='', + help='Path to directory to use for output files') + extract_parser.add_argument('paths', type=str, nargs='*', + help='Paths within file to extract (wildcard)') + extract_parser.add_argument('-U', '--uncompressed', action='store_true', + help='Output raw uncompressed data for compressed entries') + test_parser = subparsers.add_parser('test', help='Run tests') test_parser.add_argument('-P', '--processes', type=int, help='set number of processes to use for running tests') diff --git a/tools/binman/control.py b/tools/binman/control.py index b244e7a0cd0..dc898be6179 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -118,6 +118,57 @@ def ReadEntry(image_fname, entry_path, decomp=True): return entry.ReadData(decomp)

+def ExtractEntries(image_fname, output_fname, outdir, entry_paths, + decomp=True): + """Extract the data from one or more entries and write it to files + + Args: + image_fname: Image filename to process + output_fname: Single output filename to use if extracting one file, None + otherwise + outdir: Output directory to use (for any number of files), else None + entry_paths: List of entry paths to extract + decomp: True to compress the entry data + + Returns: + List of EntryInfo records that were written + """ + image = Image.FromFile(image_fname) + + # Output an entry to a single file, as a special case + if output_fname: + if not entry_paths: + raise ValueError('Must specify an entry path to write with -o') + if len(entry_paths) != 1: + raise ValueError('Must specify exactly one entry path to write with -o') + entry = image.FindEntryPath(entry_paths[0]) + data = entry.ReadData(decomp) + tools.WriteFile(output_fname, data) + tout.Notice("Wrote %#x bytes to file '%s'" % (len(data), output_fname)) + return + + # Otherwise we will output to a path given by the entry path of each entry. + # This means that entries will appear in subdirectories if they are part of + # a sub-section. + einfos = image.GetListEntries(entry_paths)[0] + tout.Notice('%d entries match and will be written' % len(einfos)) + for einfo in einfos: + entry = einfo.entry + data = entry.ReadData(decomp) + path = entry.GetPath()[1:] + fname = os.path.join(outdir, path) + + # If this entry has children, create a directory for it and put its + # data in a file called 'root' in that directory + if entry.GetEntries(): + if not os.path.exists(fname): + os.makedirs(fname) + fname = os.path.join(fname, 'root') + tout.Notice("Write entry '%s' to '%s'" % (entry.GetPath(), fname)) + tools.WriteFile(fname, data) + return einfos + + def Binman(args): """The main control code for binman

@@ -142,6 +193,15 @@ def Binman(args): ListEntries(args.image, args.paths) return 0

+ if args.cmd == 'extract': + try: + tools.PrepareOutputDir(None) + ExtractEntries(args.image, args.filename, args.outdir, args.paths, + not args.uncompressed) + finally: + tools.FinaliseOutputDir() + return 0 + # Try to figure out which device tree contains our image description if args.dt: dtb_fname = args.dt diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index c11dd1b85ab..709fa0adc3f 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -2446,6 +2446,43 @@ class TestFunctional(unittest.TestCase): data = self._RunExtractCmd('u-boot') self.assertEqual(U_BOOT_DATA, data)

+ def testExtractSection(self): + """Test extracting the files in a section""" + data = self._RunExtractCmd('section') + cbfs_data = data[:0x400] + cbfs = cbfs_util.CbfsReader(cbfs_data) + self.assertEqual(['u-boot', 'u-boot-dtb', ''], cbfs.files.keys()) + dtb_data = data[0x400:] + dtb = self._decompress(dtb_data) + self.assertEqual(EXTRACT_DTB_SIZE, len(dtb)) + + def testExtractCompressed(self): + """Test extracting compressed data""" + data = self._RunExtractCmd('section/u-boot-dtb') + self.assertEqual(EXTRACT_DTB_SIZE, len(data)) + + def testExtractRaw(self): + """Test extracting compressed data without decompressing it""" + data = self._RunExtractCmd('section/u-boot-dtb', decomp=False) + dtb = self._decompress(data) + self.assertEqual(EXTRACT_DTB_SIZE, len(dtb)) + + def testExtractCbfs(self): + """Test extracting CBFS data""" + data = self._RunExtractCmd('section/cbfs/u-boot') + self.assertEqual(U_BOOT_DATA, data) + + def testExtractCbfsCompressed(self): + """Test extracting CBFS compressed data""" + data = self._RunExtractCmd('section/cbfs/u-boot-dtb') + self.assertEqual(EXTRACT_DTB_SIZE, len(data)) + + def testExtractCbfsRaw(self): + """Test extracting CBFS compressed data without decompressing it""" + data = self._RunExtractCmd('section/cbfs/u-boot-dtb', decomp=False) + dtb = tools.Decompress(data, 'lzma') + self.assertEqual(EXTRACT_DTB_SIZE, len(dtb)) + def testExtractBadEntry(self): """Test extracting a bad section path""" with self.assertRaises(ValueError) as e: @@ -2465,6 +2502,158 @@ class TestFunctional(unittest.TestCase): with self.assertRaises(ValueError) as e: control.ReadEntry(fname, 'name')

+ def testExtractCmd(self): + """Test extracting a file fron an image on the command line""" + self._CheckLz4() + self._DoReadFileRealDtb('130_list_fdtmap.dts') + image_fname = tools.GetOutputFilename('image.bin') + fname = os.path.join(self._indir, 'output.extact') + with test_util.capture_sys_output() as (stdout, stderr): + self._DoBinman('extract', '-i', image_fname, 'u-boot', '-f', fname) + data = tools.ReadFile(fname) + self.assertEqual(U_BOOT_DATA, data) + + def testExtractOneEntry(self): + """Test extracting a single entry fron an image """ + self._CheckLz4() + self._DoReadFileRealDtb('130_list_fdtmap.dts') + image_fname = tools.GetOutputFilename('image.bin') + fname = os.path.join(self._indir, 'output.extact') + control.ExtractEntries(image_fname, fname, None, ['u-boot']) + data = tools.ReadFile(fname) + self.assertEqual(U_BOOT_DATA, data) + + def _CheckExtractOutput(self, decomp): + """Helper to test file output with and without decompression + + Args: + decomp: True to decompress entry data, False to output it raw + """ + def _CheckPresent(entry_path, expect_data, expect_size=None): + """Check and remove expected file + + This checks the data/size of a file and removes the file both from + the outfiles set and from the output directory. Once all files are + processed, both the set and directory should be empty. + + Args: + entry_path: Entry path + expect_data: Data to expect in file, or None to skip check + expect_size: Size of data to expect in file, or None to skip + """ + path = os.path.join(outdir, entry_path) + data = tools.ReadFile(path) + os.remove(path) + if expect_data: + self.assertEqual(expect_data, data) + elif expect_size: + self.assertEqual(expect_size, len(data)) + outfiles.remove(path) + + def _CheckDirPresent(name): + """Remove expected directory + + This gives an error if the directory does not exist as expected + + Args: + name: Name of directory to remove + """ + path = os.path.join(outdir, name) + os.rmdir(path) + + self._DoReadFileRealDtb('130_list_fdtmap.dts') + image_fname = tools.GetOutputFilename('image.bin') + outdir = os.path.join(self._indir, 'extract') + einfos = control.ExtractEntries(image_fname, None, outdir, [], decomp) + + # Create a set of all file that were output (should be 9) + outfiles = set() + for root, dirs, files in os.walk(outdir): + outfiles |= set([os.path.join(root, fname) for fname in files]) + self.assertEqual(9, len(outfiles)) + self.assertEqual(9, len(einfos)) + + image = control.images['image'] + entries = image.GetEntries() + + # Check the 9 files in various ways + section = entries['section'] + section_entries = section.GetEntries() + cbfs_entries = section_entries['cbfs'].GetEntries() + _CheckPresent('u-boot', U_BOOT_DATA) + _CheckPresent('section/cbfs/u-boot', U_BOOT_DATA) + dtb_len = EXTRACT_DTB_SIZE + if not decomp: + dtb_len = cbfs_entries['u-boot-dtb'].size + _CheckPresent('section/cbfs/u-boot-dtb', None, dtb_len) + if not decomp: + dtb_len = section_entries['u-boot-dtb'].size + _CheckPresent('section/u-boot-dtb', None, dtb_len) + + fdtmap = entries['fdtmap'] + _CheckPresent('fdtmap', fdtmap.data) + hdr = entries['image-header'] + _CheckPresent('image-header', hdr.data) + + _CheckPresent('section/root', section.data) + cbfs = section_entries['cbfs'] + _CheckPresent('section/cbfs/root', cbfs.data) + data = tools.ReadFile(image_fname) + _CheckPresent('root', data) + + # There should be no files left. Remove all the directories to check. + # If there are any files/dirs remaining, one of these checks will fail. + self.assertEqual(0, len(outfiles)) + _CheckDirPresent('section/cbfs') + _CheckDirPresent('section') + _CheckDirPresent('') + self.assertFalse(os.path.exists(outdir)) + + def testExtractAllEntries(self): + """Test extracting all entries""" + self._CheckLz4() + self._CheckExtractOutput(decomp=True) + + def testExtractAllEntriesRaw(self): + """Test extracting all entries without decompressing them""" + self._CheckLz4() + self._CheckExtractOutput(decomp=False) + + def testExtractSelectedEntries(self): + """Test extracting some entries""" + self._CheckLz4() + self._DoReadFileRealDtb('130_list_fdtmap.dts') + image_fname = tools.GetOutputFilename('image.bin') + outdir = os.path.join(self._indir, 'extract') + einfos = control.ExtractEntries(image_fname, None, outdir, + ['*cb*', '*head*']) + + # File output is tested by testExtractAllEntries(), so just check that + # the expected entries are selected + names = [einfo.name for einfo in einfos] + self.assertEqual(names, + ['cbfs', 'u-boot', 'u-boot-dtb', 'image-header']) + + def testExtractNoEntryPaths(self): + """Test extracting some entries""" + self._CheckLz4() + self._DoReadFileRealDtb('130_list_fdtmap.dts') + image_fname = tools.GetOutputFilename('image.bin') + with self.assertRaises(ValueError) as e: + control.ExtractEntries(image_fname, 'fname', None, []) + self.assertIn('Must specify an entry path to write with -o', + str(e.exception)) + + def testExtractTooManyEntryPaths(self): + """Test extracting some entries""" + self._CheckLz4() + self._DoReadFileRealDtb('130_list_fdtmap.dts') + image_fname = tools.GetOutputFilename('image.bin') + with self.assertRaises(ValueError) as e: + control.ExtractEntries(image_fname, 'fname', None, ['a', 'b']) + self.assertIn('Must specify exactly one entry path to write with -o', + str(e.exception)) +

if __name__ == "__main__": unittest.main()

Current test coverage is likely sufficient for the logic used to place sections in the image. However it seems useful to add a test specifically for nested sections, since these could have some unusual interactions.

Add a new test for this and aligned sections. This test failed before the refactor to drop the bsection.py file (Section class), but passes now.

Signed-off-by: Simon Glass sjg@chromium.org ---

Changes in v2: - Add patches to enhance the 'ls' command to filter which entries are shown - Add patches to implement the 'extract' command

tools/binman/ftest.py | 67 +++++++++++++++++++- tools/binman/test/131_pack_align_section.dts | 28 ++++++++ 2 files changed, 94 insertions(+), 1 deletion(-) create mode 100644 tools/binman/test/131_pack_align_section.dts

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 709fa0adc3f..6a40d1fdbb4 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -586,7 +586,7 @@ class TestFunctional(unittest.TestCase):

def testSimpleDebug(self): """Test a simple binman run with debugging enabled""" - data = self._DoTestFile('005_simple.dts', debug=True) + self._DoTestFile('005_simple.dts', debug=True)

def testDual(self): """Test that we can handle creating two images @@ -2654,6 +2654,71 @@ class TestFunctional(unittest.TestCase): self.assertIn('Must specify exactly one entry path to write with -o', str(e.exception))

+ def testPackAlignSection(self): + """Test that sections can have alignment""" + self._DoReadFile('131_pack_align_section.dts') + + self.assertIn('image', control.images) + image = control.images['image'] + entries = image.GetEntries() + self.assertEqual(3, len(entries)) + + # First u-boot + self.assertIn('u-boot', entries) + entry = entries['u-boot'] + self.assertEqual(0, entry.offset) + self.assertEqual(0, entry.image_pos) + self.assertEqual(len(U_BOOT_DATA), entry.contents_size) + self.assertEqual(len(U_BOOT_DATA), entry.size) + + # Section0 + self.assertIn('section0', entries) + section0 = entries['section0'] + self.assertEqual(0x10, section0.offset) + self.assertEqual(0x10, section0.image_pos) + self.assertEqual(len(U_BOOT_DATA), section0.size) + + # Second u-boot + section_entries = section0.GetEntries() + self.assertIn('u-boot', section_entries) + entry = section_entries['u-boot'] + self.assertEqual(0, entry.offset) + self.assertEqual(0x10, entry.image_pos) + self.assertEqual(len(U_BOOT_DATA), entry.contents_size) + self.assertEqual(len(U_BOOT_DATA), entry.size) + + # Section1 + self.assertIn('section1', entries) + section1 = entries['section1'] + self.assertEqual(0x14, section1.offset) + self.assertEqual(0x14, section1.image_pos) + self.assertEqual(0x20, section1.size) + + # Second u-boot + section_entries = section1.GetEntries() + self.assertIn('u-boot', section_entries) + entry = section_entries['u-boot'] + self.assertEqual(0, entry.offset) + self.assertEqual(0x14, entry.image_pos) + self.assertEqual(len(U_BOOT_DATA), entry.contents_size) + self.assertEqual(len(U_BOOT_DATA), entry.size) + + # Section2 + self.assertIn('section2', section_entries) + section2 = section_entries['section2'] + self.assertEqual(0x4, section2.offset) + self.assertEqual(0x18, section2.image_pos) + self.assertEqual(4, section2.size) + + # Third u-boot + section_entries = section2.GetEntries() + self.assertIn('u-boot', section_entries) + entry = section_entries['u-boot'] + self.assertEqual(0, entry.offset) + self.assertEqual(0x18, entry.image_pos) + self.assertEqual(len(U_BOOT_DATA), entry.contents_size) + self.assertEqual(len(U_BOOT_DATA), entry.size) +

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/131_pack_align_section.dts b/tools/binman/test/131_pack_align_section.dts new file mode 100644 index 00000000000..44478855b09 --- /dev/null +++ b/tools/binman/test/131_pack_align_section.dts @@ -0,0 +1,28 @@ +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + u-boot { + }; + section0 { + type = "section"; + align = <0x10>; + u-boot { + }; + }; + section1 { + type = "section"; + align-size = <0x20>; + u-boot { + }; + section2 { + type = "section"; + u-boot { + }; + }; + }; + }; +};