This should return empty contents, not leave it unset. Fix it.

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

tools/binman/etype/u_boot_ucode.py | 1 + 1 file changed, 1 insertion(+)

diff --git a/tools/binman/etype/u_boot_ucode.py b/tools/binman/etype/u_boot_ucode.py index 44622936182..b4cb8cdb6e1 100644 --- a/tools/binman/etype/u_boot_ucode.py +++ b/tools/binman/etype/u_boot_ucode.py @@ -81,6 +81,7 @@ class Entry_u_boot_ucode(Entry_blob): if fdt_entry: break if not fdt_entry: + self.data = b'' return True if not fdt_entry.ready: return False

Add in a few more details to this error message to make it easier to see what is going on.

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

tools/binman/etype/section.py | 10 ++++++---- tools/binman/ftest.py | 5 +++-- 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 327750461eb..a3e37c33c1b 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -220,10 +220,12 @@ class Entry_section(Entry): 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)) + entry.Raise('Offset %#x (%d) size %#x (%d) is outside the ' + "section '%s' starting at %#x (%d) " + 'of size %#x (%d)' % + (entry.offset, entry.offset, entry.size, entry.size, + self._node.path, self._skip_at_start, + self._skip_at_start, self.size, self.size)) if entry.offset < offset and entry.size: entry.Raise("Offset %#x (%d) overlaps with previous entry '%s' " "ending at %#x (%d)" % diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index b771b9d5df7..8e6222ad785 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -968,8 +968,9 @@ class TestFunctional(unittest.TestCase): """Test that the end-at-4gb property checks for offset boundaries""" with self.assertRaises(ValueError) as e: self._DoTestFile('028_pack_4gb_outside.dts') - self.assertIn("Node '/binman/u-boot': Offset 0x0 (0) is outside " - "the section starting at 0xffffffe0 (4294967264)", + self.assertIn("Node '/binman/u-boot': Offset 0x0 (0) size 0x4 (4) " + "is outside the section '/binman' starting at " + '0xffffffe0 (4294967264) of size 0x20 (32)', str(e.exception))

def testPackX86Rom(self):

At present we use 'compress' as the property to set the compression of a 'files' entry. But this conflicts with the same property for entries, of which Entry_section is a subclass.

Strictly speaking, since Entry_files is in fact a subclass of Entry_section, the files can be compressed individually but also the section (that contains all the files) can itself be compressed. With this change, it is possible to express that.

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

tools/binman/README.entries | 15 ++++++++++++++- tools/binman/etype/files.py | 7 ++++--- tools/binman/etype/section.py | 4 ++-- tools/binman/test/085_files_compress.dts | 2 +- 4 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/tools/binman/README.entries b/tools/binman/README.entries index c1d436563e8..a3a314753c5 100644 --- a/tools/binman/README.entries +++ b/tools/binman/README.entries @@ -299,7 +299,7 @@ Entry: files: Entry containing a set of files

Properties / Entry arguments: - pattern: Filename pattern to match the files to include - - compress: Compression algorithm to use: + - files-compress: Compression algorithm to use: none: No compression lz4: Use lz4 compression (via 'lz4' command-line utility)

@@ -406,6 +406,10 @@ The 'default' property, if present, will be automatically set to the name if of configuration whose devicetree matches the 'default-dt' entry argument, e.g. with '-a default-dt=sun50i-a64-pine64-lts'.

+Available substitutions for '@' property values are: + + DEFAULT-SEQ Sequence number of the default fdt,as provided by the + 'default-dt' entry argument

Properties (in the 'fit' node itself): fit,external-offset: Indicates that the contents of the FIT are external @@ -878,6 +882,15 @@ relocated to any address for execution.

+Entry: u-boot-env: An entry which contains a U-Boot environment +--------------------------------------------------------------- + +Properties / Entry arguments: + - filename: File containing the environment text, with each line in the + form var=value + + + Entry: u-boot-img: U-Boot legacy image --------------------------------------

diff --git a/tools/binman/etype/files.py b/tools/binman/etype/files.py index 9adb3afeb14..ce3832e3cdd 100644 --- a/tools/binman/etype/files.py +++ b/tools/binman/etype/files.py @@ -19,7 +19,7 @@ class Entry_files(Entry_section):

Properties / Entry arguments: - pattern: Filename pattern to match the files to include - - compress: Compression algorithm to use: + - files-compress: Compression algorithm to use: none: No compression lz4: Use lz4 compression (via 'lz4' command-line utility)

@@ -36,7 +36,8 @@ class Entry_files(Entry_section): self._pattern = fdt_util.GetString(self._node, 'pattern') if not self._pattern: self.Raise("Missing 'pattern' property") - self._compress = fdt_util.GetString(self._node, 'compress', 'none') + self._files_compress = fdt_util.GetString(self._node, 'files-compress', + 'none') self._require_matches = fdt_util.GetBool(self._node, 'require-matches')

@@ -53,7 +54,7 @@ class Entry_files(Entry_section): subnode = state.AddSubnode(self._node, name) state.AddString(subnode, 'type', 'blob') state.AddString(subnode, 'filename', fname) - state.AddString(subnode, 'compress', self._compress) + state.AddString(subnode, 'compress', self._files_compress)

# Read entries again, now that we have some self._ReadEntries() diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index a3e37c33c1b..9222042f5d8 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -160,7 +160,7 @@ class Entry_section(Entry): section_data += tools.GetBytes(self._pad_byte, pad) self.Detail('GetData: %d entries, total size %#x' % (len(self._entries), len(section_data))) - return section_data + return self.CompressData(section_data)

def GetOffsets(self): """Handle entries that want to set the offset/size of other entries @@ -414,7 +414,7 @@ class Entry_section(Entry): return None

def GetEntryContents(self): - """Call ObtainContents() for the section + """Call ObtainContents() for each entry in the section """ todo = self._entries.values() for passnum in range(3): diff --git a/tools/binman/test/085_files_compress.dts b/tools/binman/test/085_files_compress.dts index 847b398bf2b..5aeead2e6e9 100644 --- a/tools/binman/test/085_files_compress.dts +++ b/tools/binman/test/085_files_compress.dts @@ -5,7 +5,7 @@ binman { files { pattern = "files/*.dat"; - compress = "lz4"; + files-compress = "lz4"; }; }; };

Check the contents of each section to make sure it is actually in the right place.

Also fix a whitespace error in the .dts file.

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

tools/binman/ftest.py | 27 ++++++++++++++++++++++----- tools/binman/test/009_pack_extra.dts | 2 +- 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 8e6222ad785..643663d44ec 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -783,9 +783,8 @@ class TestFunctional(unittest.TestCase):

def testPackExtra(self): """Test that extra packing feature works as expected""" - retcode = self._DoTestFile('009_pack_extra.dts') + data = self._DoReadFile('009_pack_extra.dts')

- self.assertEqual(0, retcode) self.assertIn('image', control.images) image = control.images['image'] entries = image.GetEntries() @@ -797,30 +796,48 @@ class TestFunctional(unittest.TestCase): self.assertEqual(0, entry.offset) self.assertEqual(3, entry.pad_before) self.assertEqual(3 + 5 + len(U_BOOT_DATA), entry.size) + self.assertEqual(U_BOOT_DATA, entry.data) + self.assertEqual(tools.GetBytes(0, 3) + U_BOOT_DATA + + tools.GetBytes(0, 5), data[:entry.size]) + pos = entry.size

# Second u-boot has an aligned size, but it has no effect self.assertIn('u-boot-align-size-nop', entries) entry = entries['u-boot-align-size-nop'] - self.assertEqual(12, entry.offset) - self.assertEqual(4, entry.size) + self.assertEqual(pos, entry.offset) + self.assertEqual(len(U_BOOT_DATA), entry.size) + self.assertEqual(U_BOOT_DATA, entry.data) + self.assertEqual(U_BOOT_DATA, data[pos:pos + entry.size]) + pos += entry.size

# Third u-boot has an aligned size too self.assertIn('u-boot-align-size', entries) entry = entries['u-boot-align-size'] - self.assertEqual(16, entry.offset) + self.assertEqual(pos, entry.offset) self.assertEqual(32, entry.size) + self.assertEqual(U_BOOT_DATA, entry.data) + self.assertEqual(U_BOOT_DATA + tools.GetBytes(0, 32 - len(U_BOOT_DATA)), + data[pos:pos + entry.size]) + pos += entry.size

# Fourth u-boot has an aligned end self.assertIn('u-boot-align-end', entries) entry = entries['u-boot-align-end'] self.assertEqual(48, entry.offset) self.assertEqual(16, entry.size) + self.assertEqual(U_BOOT_DATA, entry.data[:len(U_BOOT_DATA)]) + self.assertEqual(U_BOOT_DATA + tools.GetBytes(0, 16 - len(U_BOOT_DATA)), + data[pos:pos + entry.size]) + pos += entry.size

# Fifth u-boot immediately afterwards self.assertIn('u-boot-align-both', entries) entry = entries['u-boot-align-both'] self.assertEqual(64, entry.offset) self.assertEqual(64, entry.size) + self.assertEqual(U_BOOT_DATA, entry.data[:len(U_BOOT_DATA)]) + self.assertEqual(U_BOOT_DATA + tools.GetBytes(0, 64 - len(U_BOOT_DATA)), + data[pos:pos + entry.size])

self.CheckNoGaps(entries) self.assertEqual(128, image.size) diff --git a/tools/binman/test/009_pack_extra.dts b/tools/binman/test/009_pack_extra.dts index 0765707dea2..1b315557716 100644 --- a/tools/binman/test/009_pack_extra.dts +++ b/tools/binman/test/009_pack_extra.dts @@ -28,7 +28,7 @@

u-boot-align-both { type = "u-boot"; - align= <64>; + align = <64>; align-end = <128>; }; };

Alignment does form part of the entry once the image is written out, but within binman the entry contents does not include the padding. Add documentation to make this clear, as well as a test.

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

tools/binman/README | 29 ++++++++++++++++++++--------- tools/binman/entry.py | 6 ++++-- tools/binman/ftest.py | 33 ++++++++++++++++++++++++++++++++- 3 files changed, 56 insertions(+), 12 deletions(-)

diff --git a/tools/binman/README b/tools/binman/README index 0433cabce4f..0dee71d1b22 100644 --- a/tools/binman/README +++ b/tools/binman/README @@ -278,9 +278,12 @@ offset:

align: This sets the alignment of the entry. The entry offset is adjusted - so that the entry starts on an aligned boundary within the image. For - example 'align = <16>' means that the entry will start on a 16-byte - boundary. Alignment shold be a power of 2. If 'align' is not + so that the entry starts on an aligned boundary within the containing + section or image. For example 'align = <16>' means that the entry will + start on a 16-byte boundary. This may mean that padding is added before + the entry. The padding is part of the containing section but is not + included in the entry, meaning that an empty space may be created before + the entry starts. Alignment should be a power of 2. If 'align' is not provided, no alignment is performed.

size: @@ -308,14 +311,22 @@ pad-after: align-size: This sets the alignment of the entry size. For example, to ensure that the size of an entry is a multiple of 64 bytes, set this to 64. - If 'align-size' is not provided, no alignment is performed. + While this does not affect the contents of the entry within binman + itself (the padding is performed only when its parent section is + assembled), the end result is that the entry ends with the padding + bytes, so may grow. If 'align-size' is not provided, no alignment is + performed.

align-end: - This sets the alignment of the end of an entry. Some entries require - that they end on an alignment boundary, regardless of where they - start. This does not move the start of the entry, so the contents of - the entry will still start at the beginning. But there may be padding - at the end. If 'align-end' is not provided, no alignment is performed. + This sets the alignment of the end of an entry with respect to the + containing section. Some entries require that they end on an alignment + boundary, regardless of where they start. This does not move the start + of the entry, so the contents of the entry will still start at the + beginning. But there may be padding at the end. While this does not + affect the contents of the entry within binman itself (the padding is + performed only when its parent section is assembled), the end result + is that the entry ends with the padding bytes, so may grow. + If 'align-end' is not provided, no alignment is performed.

filename: For 'blob' types this provides the filename containing the binary to diff --git a/tools/binman/entry.py b/tools/binman/entry.py index e5d0aa52bd6..0421129c031 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -48,9 +48,11 @@ class Entry(object): 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: Entry start offset alignment relative to the start of the + containing section, or None align_size: Entry size alignment, or None - align_end: Entry end offset alignment, or None + align_end: Entry end offset alignment relative to the start of the + containing section, or None pad_before: Number of pad bytes before the contents when it is placed in the containing section, 0 if none. The pad bytes become part of the entry. diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 74ee7606cca..e265941a392 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -783,7 +783,8 @@ class TestFunctional(unittest.TestCase):

def testPackExtra(self): """Test that extra packing feature works as expected""" - data = self._DoReadFile('009_pack_extra.dts') + data, _, _, out_dtb_fname = self._DoReadFileDtb('009_pack_extra.dts', + update_dtb=True)

self.assertIn('image', control.images) image = control.images['image'] @@ -842,6 +843,36 @@ class TestFunctional(unittest.TestCase): self.CheckNoGaps(entries) self.assertEqual(128, image.size)

+ dtb = fdt.Fdt(out_dtb_fname) + dtb.Scan() + props = self._GetPropTree(dtb, ['size', 'offset', 'image-pos']) + expected = { + 'image-pos': 0, + 'offset': 0, + 'size': 128, + + 'u-boot:image-pos': 0, + 'u-boot:offset': 0, + 'u-boot:size': 3 + 5 + len(U_BOOT_DATA), + + 'u-boot-align-size-nop:image-pos': 12, + 'u-boot-align-size-nop:offset': 12, + 'u-boot-align-size-nop:size': 4, + + 'u-boot-align-size:image-pos': 16, + 'u-boot-align-size:offset': 16, + 'u-boot-align-size:size': 32, + + 'u-boot-align-end:image-pos': 48, + 'u-boot-align-end:offset': 48, + 'u-boot-align-end:size': 16, + + 'u-boot-align-both:image-pos': 64, + 'u-boot-align-both:offset': 64, + 'u-boot-align-both:size': 64, + } + self.assertEqual(expected, props) + def testPackAlignPowerOf2(self): """Test that invalid entry alignment is detected""" with self.assertRaises(ValueError) as e:

At present this function does the padding needed around an entry. It is easier to understand what is going on if we have a function that returns the contents of an entry, with padding included.

Refactor the code accordingly, adding a new GetPaddedData() method.

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

tools/binman/etype/section.py | 59 +++++++++++++++++++++++++++++------ tools/binman/image.py | 2 +- 2 files changed, 51 insertions(+), 10 deletions(-)

diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index d05adf00274..f80432914f2 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -16,6 +16,7 @@ from binman.entry import Entry from dtoc import fdt_util from patman import tools from patman import tout +from patman.tools import ToHexSize

class Entry_section(Entry): @@ -144,6 +145,36 @@ class Entry_section(Entry): def ObtainContents(self): return self.GetEntryContents()

+ def GetPaddedDataForEntry(self, entry): + """Get the data for an entry including any padding + + Gets the entry data and uses the section pad-byte value to add padding + before and after as defined by the pad-before and pad-after properties. + This does not consider alignment. + + Args: + entry: Entry to check + + Returns: + Contents of the entry along with any pad bytes before and + after it (bytes) + """ + data = b'' + # Handle padding before the entry + if entry.pad_before: + data += tools.GetBytes(self._pad_byte, entry.pad_before) + + # Add in the actual entry data + data += entry.GetData() + + # Handle padding after the entry + if entry.pad_after: + data += tools.GetBytes(self._pad_byte, entry.pad_after) + + self.Detail('GetPaddedDataForEntry: size %s' % ToHexSize(self.data)) + + return data + def _BuildSectionData(self): """Build the contents of a section

@@ -158,23 +189,15 @@ class Entry_section(Entry): section_data = b''

for entry in self._entries.values(): - data = entry.GetData() + data = self.GetPaddedDataForEntry(entry) # Handle empty space before the entry pad = (entry.offset or 0) - self._skip_at_start - len(section_data) if pad > 0: section_data += tools.GetBytes(self._pad_byte, pad)

- # Handle padding before the entry - if entry.pad_before: - section_data += tools.GetBytes(self._pad_byte, entry.pad_before) - # Add in the actual entry data section_data += data

- # Handle padding after the entry - if entry.pad_after: - section_data += tools.GetBytes(self._pad_byte, entry.pad_after) - if self.size: section_data += tools.GetBytes(self._pad_byte, self.size - len(section_data)) @@ -182,6 +205,24 @@ class Entry_section(Entry): (len(self._entries), len(section_data))) return self.CompressData(section_data)

+ def GetPaddedData(self): + """Get the data for a section including any padding + + Gets the section data and uses the parent section's pad-byte value to + add padding before and after as defined by the pad-before and pad-after + properties. If this is a top-level section (i.e. an image), this is the + same as GetData(), since padding is not supported. + + This does not consider alignment. + + Returns: + Contents of the section along with any pad bytes before and + after it (bytes) + """ + if self.section: + return super().GetPaddedData() + return self.GetData() + def GetData(self): return self._BuildSectionData()

diff --git a/tools/binman/image.py b/tools/binman/image.py index a8772c3763b..d65ab887b80 100644 --- a/tools/binman/image.py +++ b/tools/binman/image.py @@ -146,7 +146,7 @@ class Image(section.Entry_section): fname = tools.GetOutputFilename(self._filename) tout.Info("Writing image to '%s'" % fname) with open(fname, 'wb') as fd: - data = self.GetData() + data = self.GetPaddedData() fd.write(data) tout.Info("Wrote %#x bytes" % len(data))

At present padding of sections is inconsistent with other entry types, in that different pad bytes are used.

When a normal entry is padded by its parent, the parent's pad byte is used. But for sections, the section's pad byte is used.

Adjust logic to always do this the same way.

Note there is still a special case in entry_Section.GetPaddedData() where an image is padded with the pad byte of the top-level section. This is necessary since otherwise there would be no way to set the pad byte of the image, without adding a top-level section to every image.

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

tools/binman/etype/section.py | 4 +-- tools/binman/ftest.py | 22 ++++++++++++++++ tools/binman/test/177_section_pad.dts | 27 ++++++++++++++++++++ tools/binman/test/178_section_align.dts | 34 +++++++++++++++++++++++++ 4 files changed, 85 insertions(+), 2 deletions(-) create mode 100644 tools/binman/test/177_section_pad.dts create mode 100644 tools/binman/test/178_section_align.dts

diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 7cbb50057a3..c423a22c80f 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -165,14 +165,14 @@ class Entry_section(Entry): data = b'' # Handle padding before the entry if entry.pad_before: - data += tools.GetBytes(pad_byte, entry.pad_before) + data += tools.GetBytes(self._pad_byte, entry.pad_before)

# Add in the actual entry data data += entry.GetData()

# Handle padding after the entry if entry.pad_after: - data += tools.GetBytes(pad_byte, entry.pad_after) + data += tools.GetBytes(self._pad_byte, entry.pad_after)

if entry.size: data += tools.GetBytes(pad_byte, entry.size - len(data)) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index e265941a392..a43f79cc248 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -3821,6 +3821,28 @@ class TestFunctional(unittest.TestCase): self.assertIn("too small to hold data (need %#x more bytes)" % short, str(e.exception))

+ def testSectionPad(self): + """Testing padding with sections""" + data = self._DoReadFile('177_section_pad.dts') + expected = (tools.GetBytes(ord('&'), 3) + + tools.GetBytes(ord('!'), 5) + + U_BOOT_DATA + + tools.GetBytes(ord('!'), 1) + + tools.GetBytes(ord('&'), 2)) + self.assertEqual(expected, data) + + def testSectionAlign(self): + """Testing alignment with sections""" + data = self._DoReadFileDtb('178_section_align.dts', map=True)[0] + expected = (b'\0' + # fill section + tools.GetBytes(ord('&'), 1) + # padding to section align + b'\0' + # fill section + tools.GetBytes(ord('!'), 3) + # padding to u-boot align + U_BOOT_DATA + + tools.GetBytes(ord('!'), 4) + # padding to u-boot size + tools.GetBytes(ord('!'), 4)) # padding to section size + self.assertEqual(expected, data) +

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/177_section_pad.dts b/tools/binman/test/177_section_pad.dts new file mode 100644 index 00000000000..7e4ebf257b8 --- /dev/null +++ b/tools/binman/test/177_section_pad.dts @@ -0,0 +1,27 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + pad-byte = <0x26>; + section@0 { + read-only; + + /* Padding for the section uses the 0x26 pad byte */ + pad-before = <3>; + pad-after = <2>; + + /* Set the padding byte for entries, i.e. u-boot */ + pad-byte = <0x21>; + + u-boot { + pad-before = <5>; + pad-after = <1>; + }; + }; + }; +}; diff --git a/tools/binman/test/178_section_align.dts b/tools/binman/test/178_section_align.dts new file mode 100644 index 00000000000..90795d131b0 --- /dev/null +++ b/tools/binman/test/178_section_align.dts @@ -0,0 +1,34 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + pad-byte = <0x26>; + fill { + size = <1>; + }; + section@1 { + read-only; + + /* Padding for the section uses the 0x26 pad byte */ + align = <2>; + align-size = <0x10>; + + /* Set the padding byte for entries, i.e. u-boot */ + pad-byte = <0x21>; + + fill { + size = <1>; + }; + + u-boot { + align = <4>; + align-size = <8>; + }; + }; + }; +};

Section contents is not set up when ObtainContents() is called, since packing often changes the layout of the contents. Ensure that the contents are correctly recorded by making this function regenerate the section. It is normally only called by the parent section (when packing) or by the top-level image code, when writing out the image. So the performance impact is fairly small.

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

tools/binman/entry.py | 6 ++++++ tools/binman/etype/section.py | 14 +++++++++++++- tools/binman/ftest.py | 2 ++ 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/tools/binman/entry.py b/tools/binman/entry.py index d701eaff8fd..01a5fde84ed 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -437,6 +437,12 @@ class Entry(object): return self._node.path

def GetData(self): + """Get the contents of an entry + + Returns: + bytes content of the entry, excluding any padding. If the entry is + compressed, the compressed data is returned + """ self.Detail('GetData: size %s' % ToHexSize(self.data)) return self.data

diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index c423a22c80f..6e6f6749727 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -226,7 +226,19 @@ class Entry_section(Entry): return section.GetPaddedDataForEntry(self)

def GetData(self): - return self._BuildSectionData() + """Get the contents of an entry + + This builds the contents of the section, stores this as the contents of + the section and returns it + + Returns: + bytes content of the section, made up for all all of its subentries. + This excludes any padding. If the section is compressed, the + compressed data is returned + """ + data = self._BuildSectionData() + self.SetContents(data) + return data

def GetOffsets(self): """Handle entries that want to set the offset/size of other entries diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 7360e2ebaba..bcd4482717a 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -1820,6 +1820,8 @@ class TestFunctional(unittest.TestCase): orig = self._decompress(entry.data) self.assertEqual(orig, entry.uncomp_data)

+ self.assertEqual(image.data, entry.data) + expected = { 'blob:uncomp-size': len(COMPRESS_DATA), 'blob:size': len(data),

At present sorting and expanding entries are side-effects of the CheckEntries() function. This is a bit confusing, as 'checking' would not normally involve making changes.

Move these steps into the Pack() function instead.

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

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

diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index fb4bf640bfb..c883f0d67c4 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -260,6 +260,10 @@ class Entry_section(Entry): def Pack(self, offset): """Pack all entries into the section""" self._PackEntries() + if self._sort: + self._SortEntries() + self._ExpandEntries() + return super().Pack(offset)

def _PackEntries(self): @@ -290,9 +294,6 @@ class Entry_section(Entry):

def CheckEntries(self): """Check that entries do not overlap or extend outside the section""" - if self._sort: - self._SortEntries() - self._ExpandEntries() offset = 0 prev_name = 'None' for entry in self._entries.values():

At present CheckSize() is called from the function that packs the entries. Move it up to the main Pack() function so that _PackEntries() can just do the packing.

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

tools/binman/etype/section.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index c883f0d67c4..f93469a170b 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -264,6 +264,9 @@ class Entry_section(Entry): self._SortEntries() self._ExpandEntries()

+ size = self.CheckSize() + self.size = size + return super().Pack(offset)

def _PackEntries(self): @@ -271,7 +274,7 @@ class Entry_section(Entry): offset = self._skip_at_start for entry in self._entries.values(): offset = entry.Pack(offset) - self.size = self.CheckSize() + return offset

def _ExpandEntries(self): """Expand any entries that are permitted to"""

At present this function assumes that the size of a section is at least as large as its contents. With compression this is often not the case. Relax this constraint by using the uncompressed size, if available.

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

tools/binman/etype/section.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 1618bebe4be..b146239b779 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -299,19 +299,21 @@ class Entry_section(Entry):

def CheckEntries(self): """Check that entries do not overlap or extend outside the section""" + max_size = self.size if self.uncomp_size is None else self.uncomp_size + offset = 0 prev_name = 'None' for entry in self._entries.values(): entry.CheckEntries() if (entry.offset < self._skip_at_start or entry.offset + entry.size > self._skip_at_start + - self.size): + max_size): entry.Raise('Offset %#x (%d) size %#x (%d) is outside the ' "section '%s' starting at %#x (%d) " 'of size %#x (%d)' % (entry.offset, entry.offset, entry.size, entry.size, self._node.path, self._skip_at_start, - self._skip_at_start, self.size, self.size)) + self._skip_at_start, max_size, max_size)) if entry.offset < offset and entry.size: entry.Raise("Offset %#x (%d) overlaps with previous entry '%s' " "ending at %#x (%d)" %

With the previous changes, it is now possible to compress entire sections. Add some tests to check that compression works correctly, including updating the metadata.

Also update the documentation.

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

tools/binman/README | 8 + tools/binman/ftest.py | 217 ++++++++++++++++++ tools/binman/test/179_compress_image.dts | 14 ++ tools/binman/test/180_compress_image_less.dts | 14 ++ .../binman/test/181_compress_section_size.dts | 17 ++ tools/binman/test/182_compress_section.dts | 16 ++ tools/binman/test/183_compress_extra.dts | 37 +++ 7 files changed, 323 insertions(+) create mode 100644 tools/binman/test/179_compress_image.dts create mode 100644 tools/binman/test/180_compress_image_less.dts create mode 100644 tools/binman/test/181_compress_section_size.dts create mode 100644 tools/binman/test/182_compress_section.dts create mode 100644 tools/binman/test/183_compress_extra.dts

diff --git a/tools/binman/README b/tools/binman/README index c14cee5d115..de1eedfc3f7 100644 --- a/tools/binman/README +++ b/tools/binman/README @@ -853,6 +853,14 @@ The entry will then contain the compressed data, using the 'lz4' compression 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.

+Compression is also supported for sections. In that case the entire section is +compressed in one block, including all its contents. This means that accessing +an entry from the section required decompressing the entire section. Also, the +size of a section indicates the space that it consumes in its parent section +(and typically the image). With compression, the section may contain more data, +and the uncomp-size property indicates that, as above. The contents of the +section is compressed first, before any padding is added. This ensures that the +padding itself is not compressed, which would be a waste of time.

Map files diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 8b7537e4eca..403cd836b71 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -70,6 +70,7 @@ VBLOCK_DATA = b'vblk' FILES_DATA = (b"sorry I'm late\nOh, don't bother apologising, I'm " + b"sorry you're alive\n") COMPRESS_DATA = b'compress xxxxxxxxxxxxxxxxxxxxxx data' +COMPRESS_DATA_BIG = COMPRESS_DATA * 2 REFCODE_DATA = b'refcode' FSP_M_DATA = b'fsp_m' FSP_S_DATA = b'fsp_s' @@ -174,6 +175,7 @@ class TestFunctional(unittest.TestCase): os.path.join(cls._indir, 'files'))

TestFunctional._MakeInputFile('compress', COMPRESS_DATA) + TestFunctional._MakeInputFile('compress_big', COMPRESS_DATA_BIG) TestFunctional._MakeInputFile('bl31.bin', ATF_BL31_DATA)

# Add a few .dtb files for testing @@ -3857,6 +3859,221 @@ class TestFunctional(unittest.TestCase): tools.GetBytes(ord('!'), 4)) # padding to section size self.assertEqual(expected, data)

+ def testCompressImage(self): + """Test compression of the entire image""" + self._CheckLz4() + data, _, _, out_dtb_fname = self._DoReadFileDtb( + '179_compress_image.dts', use_real_dtb=True, update_dtb=True) + dtb = fdt.Fdt(out_dtb_fname) + dtb.Scan() + props = self._GetPropTree(dtb, ['offset', 'image-pos', 'size', + 'uncomp-size']) + orig = self._decompress(data) + self.assertEquals(COMPRESS_DATA + U_BOOT_DATA, orig) + + # Do a sanity check on various fields + image = control.images['image'] + entries = image.GetEntries() + self.assertEqual(2, len(entries)) + + entry = entries['blob'] + self.assertEqual(COMPRESS_DATA, entry.data) + self.assertEqual(len(COMPRESS_DATA), entry.size) + + entry = entries['u-boot'] + self.assertEqual(U_BOOT_DATA, entry.data) + self.assertEqual(len(U_BOOT_DATA), entry.size) + + self.assertEqual(len(data), image.size) + self.assertEqual(COMPRESS_DATA + U_BOOT_DATA, image.uncomp_data) + self.assertEqual(len(COMPRESS_DATA + U_BOOT_DATA), image.uncomp_size) + orig = self._decompress(image.data) + self.assertEqual(orig, image.uncomp_data) + + expected = { + 'blob:offset': 0, + 'blob:size': len(COMPRESS_DATA), + 'u-boot:offset': len(COMPRESS_DATA), + 'u-boot:size': len(U_BOOT_DATA), + 'uncomp-size': len(COMPRESS_DATA + U_BOOT_DATA), + 'offset': 0, + 'image-pos': 0, + 'size': len(data), + } + self.assertEqual(expected, props) + + def testCompressImageLess(self): + """Test compression where compression reduces the image size""" + self._CheckLz4() + data, _, _, out_dtb_fname = self._DoReadFileDtb( + '180_compress_image_less.dts', use_real_dtb=True, update_dtb=True) + dtb = fdt.Fdt(out_dtb_fname) + dtb.Scan() + props = self._GetPropTree(dtb, ['offset', 'image-pos', 'size', + 'uncomp-size']) + orig = self._decompress(data) + + self.assertEquals(COMPRESS_DATA + COMPRESS_DATA + U_BOOT_DATA, orig) + + # Do a sanity check on various fields + image = control.images['image'] + entries = image.GetEntries() + self.assertEqual(2, len(entries)) + + entry = entries['blob'] + self.assertEqual(COMPRESS_DATA_BIG, entry.data) + self.assertEqual(len(COMPRESS_DATA_BIG), entry.size) + + entry = entries['u-boot'] + self.assertEqual(U_BOOT_DATA, entry.data) + self.assertEqual(len(U_BOOT_DATA), entry.size) + + self.assertEqual(len(data), image.size) + self.assertEqual(COMPRESS_DATA_BIG + U_BOOT_DATA, image.uncomp_data) + self.assertEqual(len(COMPRESS_DATA_BIG + U_BOOT_DATA), + image.uncomp_size) + orig = self._decompress(image.data) + self.assertEqual(orig, image.uncomp_data) + + expected = { + 'blob:offset': 0, + 'blob:size': len(COMPRESS_DATA_BIG), + 'u-boot:offset': len(COMPRESS_DATA_BIG), + 'u-boot:size': len(U_BOOT_DATA), + 'uncomp-size': len(COMPRESS_DATA_BIG + U_BOOT_DATA), + 'offset': 0, + 'image-pos': 0, + 'size': len(data), + } + self.assertEqual(expected, props) + + def testCompressSectionSize(self): + """Test compression of a section with a fixed size""" + self._CheckLz4() + data, _, _, out_dtb_fname = self._DoReadFileDtb( + '181_compress_section_size.dts', use_real_dtb=True, update_dtb=True) + dtb = fdt.Fdt(out_dtb_fname) + dtb.Scan() + props = self._GetPropTree(dtb, ['offset', 'image-pos', 'size', + 'uncomp-size']) + orig = self._decompress(data) + self.assertEquals(COMPRESS_DATA + U_BOOT_DATA, orig) + expected = { + 'section/blob:offset': 0, + 'section/blob:size': len(COMPRESS_DATA), + 'section/u-boot:offset': len(COMPRESS_DATA), + 'section/u-boot:size': len(U_BOOT_DATA), + 'section:offset': 0, + 'section:image-pos': 0, + 'section:uncomp-size': len(COMPRESS_DATA + U_BOOT_DATA), + 'section:size': 0x30, + 'offset': 0, + 'image-pos': 0, + 'size': 0x30, + } + self.assertEqual(expected, props) + + def testCompressSection(self): + """Test compression of a section with no fixed size""" + self._CheckLz4() + data, _, _, out_dtb_fname = self._DoReadFileDtb( + '182_compress_section.dts', use_real_dtb=True, update_dtb=True) + dtb = fdt.Fdt(out_dtb_fname) + dtb.Scan() + props = self._GetPropTree(dtb, ['offset', 'image-pos', 'size', + 'uncomp-size']) + orig = self._decompress(data) + self.assertEquals(COMPRESS_DATA + U_BOOT_DATA, orig) + expected = { + 'section/blob:offset': 0, + 'section/blob:size': len(COMPRESS_DATA), + 'section/u-boot:offset': len(COMPRESS_DATA), + 'section/u-boot:size': len(U_BOOT_DATA), + 'section:offset': 0, + 'section:image-pos': 0, + 'section:uncomp-size': len(COMPRESS_DATA + U_BOOT_DATA), + 'section:size': len(data), + 'offset': 0, + 'image-pos': 0, + 'size': len(data), + } + self.assertEqual(expected, props) + + def testCompressExtra(self): + """Test compression of a section with no fixed size""" + self._CheckLz4() + data, _, _, out_dtb_fname = self._DoReadFileDtb( + '183_compress_extra.dts', use_real_dtb=True, update_dtb=True) + dtb = fdt.Fdt(out_dtb_fname) + dtb.Scan() + props = self._GetPropTree(dtb, ['offset', 'image-pos', 'size', + 'uncomp-size']) + + base = data[len(U_BOOT_DATA):] + self.assertEquals(U_BOOT_DATA, base[:len(U_BOOT_DATA)]) + rest = base[len(U_BOOT_DATA):] + + # Check compressed data + section1 = self._decompress(rest) + expect1 = tools.Compress(COMPRESS_DATA + U_BOOT_DATA, 'lz4') + self.assertEquals(expect1, rest[:len(expect1)]) + self.assertEquals(COMPRESS_DATA + U_BOOT_DATA, section1) + rest1 = rest[len(expect1):] + + section2 = self._decompress(rest1) + expect2 = tools.Compress(COMPRESS_DATA + COMPRESS_DATA, 'lz4') + self.assertEquals(expect2, rest1[:len(expect2)]) + self.assertEquals(COMPRESS_DATA + COMPRESS_DATA, section2) + rest2 = rest1[len(expect2):] + + expect_size = (len(U_BOOT_DATA) + len(U_BOOT_DATA) + len(expect1) + + len(expect2) + len(U_BOOT_DATA)) + #self.assertEquals(expect_size, len(data)) + + #self.assertEquals(U_BOOT_DATA, rest2) + + self.maxDiff = None + expected = { + 'u-boot:offset': 0, + 'u-boot:image-pos': 0, + 'u-boot:size': len(U_BOOT_DATA), + + 'base:offset': len(U_BOOT_DATA), + 'base:image-pos': len(U_BOOT_DATA), + 'base:size': len(data) - len(U_BOOT_DATA), + 'base/u-boot:offset': 0, + 'base/u-boot:image-pos': len(U_BOOT_DATA), + 'base/u-boot:size': len(U_BOOT_DATA), + 'base/u-boot2:offset': len(U_BOOT_DATA) + len(expect1) + + len(expect2), + 'base/u-boot2:image-pos': len(U_BOOT_DATA) * 2 + len(expect1) + + len(expect2), + 'base/u-boot2:size': len(U_BOOT_DATA), + + 'base/section:offset': len(U_BOOT_DATA), + 'base/section:image-pos': len(U_BOOT_DATA) * 2, + 'base/section:size': len(expect1), + 'base/section:uncomp-size': len(COMPRESS_DATA + U_BOOT_DATA), + 'base/section/blob:offset': 0, + 'base/section/blob:size': len(COMPRESS_DATA), + 'base/section/u-boot:offset': len(COMPRESS_DATA), + 'base/section/u-boot:size': len(U_BOOT_DATA), + + 'base/section2:offset': len(U_BOOT_DATA) + len(expect1), + 'base/section2:image-pos': len(U_BOOT_DATA) * 2 + len(expect1), + 'base/section2:size': len(expect2), + 'base/section2:uncomp-size': len(COMPRESS_DATA + COMPRESS_DATA), + 'base/section2/blob:offset': 0, + 'base/section2/blob:size': len(COMPRESS_DATA), + 'base/section2/blob2:offset': len(COMPRESS_DATA), + 'base/section2/blob2:size': len(COMPRESS_DATA), + + 'offset': 0, + 'image-pos': 0, + 'size': len(data), + } + self.assertEqual(expected, props) +

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/179_compress_image.dts b/tools/binman/test/179_compress_image.dts new file mode 100644 index 00000000000..4176b7f2e62 --- /dev/null +++ b/tools/binman/test/179_compress_image.dts @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: GPL-2.0+ +/dts-v1/; + +/ { + binman { + compress = "lz4"; + blob { + filename = "compress"; + }; + + u-boot { + }; + }; +}; diff --git a/tools/binman/test/180_compress_image_less.dts b/tools/binman/test/180_compress_image_less.dts new file mode 100644 index 00000000000..1d9d57b78c9 --- /dev/null +++ b/tools/binman/test/180_compress_image_less.dts @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: GPL-2.0+ +/dts-v1/; + +/ { + binman { + compress = "lz4"; + blob { + filename = "compress_big"; + }; + + u-boot { + }; + }; +}; diff --git a/tools/binman/test/181_compress_section_size.dts b/tools/binman/test/181_compress_section_size.dts new file mode 100644 index 00000000000..95ed30add1a --- /dev/null +++ b/tools/binman/test/181_compress_section_size.dts @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: GPL-2.0+ +/dts-v1/; + +/ { + binman { + section { + size = <0x30>; + compress = "lz4"; + blob { + filename = "compress"; + }; + + u-boot { + }; + }; + }; +}; diff --git a/tools/binman/test/182_compress_section.dts b/tools/binman/test/182_compress_section.dts new file mode 100644 index 00000000000..dc3e340c5d6 --- /dev/null +++ b/tools/binman/test/182_compress_section.dts @@ -0,0 +1,16 @@ +// SPDX-License-Identifier: GPL-2.0+ +/dts-v1/; + +/ { + binman { + section { + compress = "lz4"; + blob { + filename = "compress"; + }; + + u-boot { + }; + }; + }; +}; diff --git a/tools/binman/test/183_compress_extra.dts b/tools/binman/test/183_compress_extra.dts new file mode 100644 index 00000000000..59aae822638 --- /dev/null +++ b/tools/binman/test/183_compress_extra.dts @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: GPL-2.0+ +/dts-v1/; + +/ { + binman { + u-boot { + }; + base { + type = "section"; + u-boot { + }; + section { + compress = "lz4"; + blob { + filename = "compress"; + }; + + u-boot { + }; + }; + section2 { + type = "section"; + compress = "lz4"; + blob { + filename = "compress"; + }; + blob2 { + type = "blob"; + filename = "compress"; + }; + }; + u-boot2 { + type = "u-boot"; + }; + }; + }; +};