| Message ID | 20220808105125.21356-2-stefan.herbrechtsmeier-oss@weidmueller.com |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [v2,01/10] binman: Skip elf tests if python elftools is not available | expand |
Hi Stefan, On Mon, 8 Aug 2022 at 04:51, Stefan Herbrechtsmeier <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: > > From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > > Move the compressed data header handling into the dtb blob entry class > and make it optional. The header is uncommon, not supported by U-Boot > and incompatible with external compressed artifacts. > > If needed the header could be enabled with the following > attribute beside the compress attribute: > prepend = "length"; > > The header was introduced as part of commit eb0f4a4cb402 ("binman: > Support replacing data in a cbfs") to allow device tree entries to be > larger that the compressed contents. Regarding the commit "this is > necessary to cope with a compressed device tree being updated in such a > way that it shrinks after the entry size is already set (an obscure > case)". This case need to be fixed without influence any compressed data > by itself. > > Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > > --- > > Changes in v2: > - Reworked to make the feature optional. > > tools/binman/cbfs_util.py | 8 ++--- > tools/binman/comp_util.py | 11 ++---- > tools/binman/entries.rst | 4 +++ > tools/binman/entry.py | 2 +- > tools/binman/etype/blob_dtb.py | 21 ++++++++++++ > tools/binman/ftest.py | 34 ++++++++++++++++--- > .../test/235_compress_prepend_length_dtb.dts | 17 ++++++++++ > 7 files changed, 78 insertions(+), 19 deletions(-) > create mode 100644 tools/binman/test/235_compress_prepend_length_dtb.dts I've been through this and I think it is a good change. We can use your technique (property in the blob_dtb etype) to deal with any future problems that come up. But I would like to split this patch into three: 1. Add your blob_dtb property and the test 2. Change all uses of compress()/decompress() to call with add with_header=False 3. Drop the with_header arg from comp_util.py A few more tweaks below. > > diff --git a/tools/binman/cbfs_util.py b/tools/binman/cbfs_util.py > index 9cad03886f..a1836f4ad3 100644 > --- a/tools/binman/cbfs_util.py > +++ b/tools/binman/cbfs_util.py > @@ -241,9 +241,9 @@ class CbfsFile(object): > """Handle decompressing data if necessary""" > indata = self.data > if self.compress == COMPRESS_LZ4: > - data = comp_util.decompress(indata, 'lz4', with_header=False) > + data = comp_util.decompress(indata, 'lz4') > elif self.compress == COMPRESS_LZMA: > - data = comp_util.decompress(indata, 'lzma', with_header=False) > + data = comp_util.decompress(indata, 'lzma') > else: > data = indata > self.memlen = len(data) > @@ -362,9 +362,9 @@ class CbfsFile(object): > elif self.ftype == TYPE_RAW: > orig_data = data > if self.compress == COMPRESS_LZ4: > - data = comp_util.compress(orig_data, 'lz4', with_header=False) > + data = comp_util.compress(orig_data, 'lz4') > elif self.compress == COMPRESS_LZMA: > - data = comp_util.compress(orig_data, 'lzma', with_header=False) > + data = comp_util.compress(orig_data, 'lzma') > self.memlen = len(orig_data) > self.data_len = len(data) > attr = struct.pack(ATTR_COMPRESSION_FORMAT, > diff --git a/tools/binman/comp_util.py b/tools/binman/comp_util.py > index dc76adab35..269bbf7975 100644 > --- a/tools/binman/comp_util.py > +++ b/tools/binman/comp_util.py > @@ -3,7 +3,6 @@ > # > """Utilities to compress and decompress data""" > > -import struct > import tempfile > > from binman import bintool > @@ -16,7 +15,7 @@ LZMA_ALONE = bintool.Bintool.create('lzma_alone') > HAVE_LZMA_ALONE = LZMA_ALONE.is_present() > > > -def compress(indata, algo, with_header=True): > +def compress(indata, algo): > """Compress some data using a given algorithm > > Note that for lzma this uses an old version of the algorithm, not that > @@ -41,12 +40,9 @@ def compress(indata, algo, with_header=True): > data = LZMA_ALONE.compress(indata) > else: > raise ValueError("Unknown algorithm '%s'" % algo) > - if with_header: > - hdr = struct.pack('<I', len(data)) > - data = hdr + data > return data > > -def decompress(indata, algo, with_header=True): > +def decompress(indata, algo): > """Decompress some data using a given algorithm > > Note that for lzma this uses an old version of the algorithm, not that > @@ -64,9 +60,6 @@ def decompress(indata, algo, with_header=True): > """ > if algo == 'none': > return indata > - if with_header: > - data_len = struct.unpack('<I', indata[:4])[0] > - indata = indata[4:4 + data_len] > if algo == 'lz4': > data = LZ4.decompress(indata) > elif algo == 'lzma': > diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst > index ae4305c99e..8e722426d3 100644 > --- a/tools/binman/entries.rst > +++ b/tools/binman/entries.rst > @@ -208,6 +208,10 @@ This is a blob containing a device tree. The contents of the blob are > obtained from the list of available device-tree files, managed by the > 'state' module. > > +Additional Properties / Entry arguments: > + - prepend: Header type to use: > + none: No header > + length: 32-bit length header > > > Entry: blob-ext: Externally built binary blob > diff --git a/tools/binman/entry.py b/tools/binman/entry.py > index a07a588864..8cbfd43af9 100644 > --- a/tools/binman/entry.py > +++ b/tools/binman/entry.py > @@ -1069,7 +1069,7 @@ features to produce new behaviours. > indata: Data to compress > > Returns: > - Compressed data (first word is the compressed size) > + Compressed data > """ > self.uncomp_data = indata > if self.compress != 'none': > diff --git a/tools/binman/etype/blob_dtb.py b/tools/binman/etype/blob_dtb.py > index 4159e3032a..652b8abd8f 100644 > --- a/tools/binman/etype/blob_dtb.py > +++ b/tools/binman/etype/blob_dtb.py > @@ -7,6 +7,8 @@ > > from binman.entry import Entry > from binman.etype.blob import Entry_blob > +from dtoc import fdt_util > +import struct > > # This is imported if needed > state = None > @@ -17,6 +19,9 @@ class Entry_blob_dtb(Entry_blob): > This is a blob containing a device tree. The contents of the blob are > obtained from the list of available device-tree files, managed by the > 'state' module. > + > + Additional attributes: > + prepend: Header used (e.g. 'length'), 'none' if none > """ > def __init__(self, section, etype, node): > # Put this here to allow entry-docs and help to work without libfdt > @@ -25,6 +30,12 @@ class Entry_blob_dtb(Entry_blob): > > super().__init__(section, etype, node) > > + self.prepend = 'none' None ? > + > + def ReadNode(self): > + super().ReadNode() > + self.prepend = fdt_util.GetString(self._node, 'prepend', 'none') Can you drop the 'none' so that it uses None instead? Aso we should check for a valid value here - e.g. it must be 'length' and not something else, otherwise self.Raise() > + > def ObtainContents(self): > """Get the device-tree from the list held by the 'state' module""" > self._filename = self.GetDefaultFilename() > @@ -35,6 +46,9 @@ class Entry_blob_dtb(Entry_blob): > """Re-read the DTB contents so that we get any calculated properties""" > _, indata = state.GetFdtContents(self.GetFdtEtype()) > data = self.CompressData(indata) > + if self.prepend == 'length': > + hdr = struct.pack('<I', len(data)) > + data = hdr + data > return self.ProcessContentsUpdate(data) > > def GetFdtEtype(self): > @@ -50,6 +64,13 @@ class Entry_blob_dtb(Entry_blob): > fname = self.GetDefaultFilename() > return {self.GetFdtEtype(): [self, fname]} > > + def ReadData(self, decomp=True, alt_format=None): > + data = super().ReadData(decomp, alt_format) > + if self.prepend == 'length': > + data_len = struct.unpack('<I', data[:4])[0] > + data = data[4:4 + data_len] > + return data > + > def WriteData(self, data, decomp=True): > ok = super().WriteData(data, decomp) > > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py > index da9aa9e679..057b4e28b7 100644 > --- a/tools/binman/ftest.py > +++ b/tools/binman/ftest.py > @@ -2536,6 +2536,28 @@ class TestFunctional(unittest.TestCase): > } > self.assertEqual(expected, props) > > + def testCompressPrependLengthDtb(self): > + """Test that compress of device-tree files with length header is > + supported > + """ > + data = self.data = self._DoReadFileRealDtb('235_compress_prepend_length_dtb.dts') > + self.assertEqual(U_BOOT_DATA, data[:len(U_BOOT_DATA)]) > + dtb_data = data[len(U_BOOT_DATA):] > + comp_data_len = struct.unpack('<I', dtb_data[:4])[0] > + comp_data = dtb_data[4:4 + comp_data_len] > + 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(dtb_data), > + 'size': len(data), > + } > + self.assertEqual(expected, props) > + > + > def testCbfsUpdateFdt(self): > """Test that we can update the device tree with CBFS offset/size info""" > self._CheckLz4() > @@ -2856,7 +2878,7 @@ class TestFunctional(unittest.TestCase): > def testExtractCbfsRaw(self): > """Test extracting CBFS compressed data without decompressing it""" > data = self._RunExtractCmd('section/cbfs/u-boot-dtb', decomp=False) > - dtb = comp_util.decompress(data, 'lzma', with_header=False) > + dtb = comp_util.decompress(data, 'lzma') > self.assertEqual(EXTRACT_DTB_SIZE, len(dtb)) > > def testExtractBadEntry(self): > @@ -4427,15 +4449,17 @@ class TestFunctional(unittest.TestCase): > rest = base[len(U_BOOT_DATA):] > > # Check compressed data > - section1 = self._decompress(rest) > expect1 = comp_util.compress(COMPRESS_DATA + U_BOOT_DATA, 'lz4') > - self.assertEquals(expect1, rest[:len(expect1)]) > + data1 = rest[:len(expect1)] > + section1 = self._decompress(data1) > + self.assertEquals(expect1, data1) > self.assertEquals(COMPRESS_DATA + U_BOOT_DATA, section1) > rest1 = rest[len(expect1):] > > - section2 = self._decompress(rest1) > expect2 = comp_util.compress(COMPRESS_DATA + COMPRESS_DATA, 'lz4') > - self.assertEquals(expect2, rest1[:len(expect2)]) > + data2 = rest1[:len(expect2)] > + section2 = self._decompress(data2) > + self.assertEquals(expect2, data2) > self.assertEquals(COMPRESS_DATA + COMPRESS_DATA, section2) > rest2 = rest1[len(expect2):] > > diff --git a/tools/binman/test/235_compress_prepend_length_dtb.dts b/tools/binman/test/235_compress_prepend_length_dtb.dts > new file mode 100644 > index 0000000000..a5abc60477 > --- /dev/null > +++ b/tools/binman/test/235_compress_prepend_length_dtb.dts > @@ -0,0 +1,17 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +/dts-v1/; > + > +/ { > + #address-cells = <1>; > + #size-cells = <1>; > + > + binman { > + u-boot { > + }; > + u-boot-dtb { > + compress = "lz4"; > + prepend = "length"; > + }; > + }; > +}; > -- > 2.30.2 > Regards, Simon
Hi Simon, Am 13.08.2022 um 16:59 schrieb Simon Glass: > Hi Stefan, > > On Mon, 8 Aug 2022 at 04:51, Stefan Herbrechtsmeier > <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: >> >> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> >> >> Move the compressed data header handling into the dtb blob entry class >> and make it optional. The header is uncommon, not supported by U-Boot >> and incompatible with external compressed artifacts. >> >> If needed the header could be enabled with the following >> attribute beside the compress attribute: >> prepend = "length"; >> >> The header was introduced as part of commit eb0f4a4cb402 ("binman: >> Support replacing data in a cbfs") to allow device tree entries to be >> larger that the compressed contents. Regarding the commit "this is >> necessary to cope with a compressed device tree being updated in such a >> way that it shrinks after the entry size is already set (an obscure >> case)". This case need to be fixed without influence any compressed data >> by itself. >> >> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> >> >> --- >> >> Changes in v2: >> - Reworked to make the feature optional. >> >> tools/binman/cbfs_util.py | 8 ++--- >> tools/binman/comp_util.py | 11 ++---- >> tools/binman/entries.rst | 4 +++ >> tools/binman/entry.py | 2 +- >> tools/binman/etype/blob_dtb.py | 21 ++++++++++++ >> tools/binman/ftest.py | 34 ++++++++++++++++--- >> .../test/235_compress_prepend_length_dtb.dts | 17 ++++++++++ >> 7 files changed, 78 insertions(+), 19 deletions(-) >> create mode 100644 tools/binman/test/235_compress_prepend_length_dtb.dts > > I've been through this and I think it is a good change. We can use > your technique (property in the blob_dtb etype) to deal with any > future problems that come up. > > But I would like to split this patch into three: > > 1. Add your blob_dtb property and the test > 2. Change all uses of compress()/decompress() to call with add with_header=False > 3. Drop the with_header arg from comp_util.py Okay, I will split the commit. > > A few more tweaks below. > >> >> diff --git a/tools/binman/cbfs_util.py b/tools/binman/cbfs_util.py >> index 9cad03886f..a1836f4ad3 100644 >> --- a/tools/binman/cbfs_util.py >> +++ b/tools/binman/cbfs_util.py >> @@ -241,9 +241,9 @@ class CbfsFile(object): >> """Handle decompressing data if necessary""" >> indata = self.data >> if self.compress == COMPRESS_LZ4: >> - data = comp_util.decompress(indata, 'lz4', with_header=False) >> + data = comp_util.decompress(indata, 'lz4') >> elif self.compress == COMPRESS_LZMA: >> - data = comp_util.decompress(indata, 'lzma', with_header=False) >> + data = comp_util.decompress(indata, 'lzma') >> else: >> data = indata >> self.memlen = len(data) >> @@ -362,9 +362,9 @@ class CbfsFile(object): >> elif self.ftype == TYPE_RAW: >> orig_data = data >> if self.compress == COMPRESS_LZ4: >> - data = comp_util.compress(orig_data, 'lz4', with_header=False) >> + data = comp_util.compress(orig_data, 'lz4') >> elif self.compress == COMPRESS_LZMA: >> - data = comp_util.compress(orig_data, 'lzma', with_header=False) >> + data = comp_util.compress(orig_data, 'lzma') >> self.memlen = len(orig_data) >> self.data_len = len(data) >> attr = struct.pack(ATTR_COMPRESSION_FORMAT, >> diff --git a/tools/binman/comp_util.py b/tools/binman/comp_util.py >> index dc76adab35..269bbf7975 100644 >> --- a/tools/binman/comp_util.py >> +++ b/tools/binman/comp_util.py >> @@ -3,7 +3,6 @@ >> # >> """Utilities to compress and decompress data""" >> >> -import struct >> import tempfile >> >> from binman import bintool >> @@ -16,7 +15,7 @@ LZMA_ALONE = bintool.Bintool.create('lzma_alone') >> HAVE_LZMA_ALONE = LZMA_ALONE.is_present() >> >> >> -def compress(indata, algo, with_header=True): >> +def compress(indata, algo): >> """Compress some data using a given algorithm >> >> Note that for lzma this uses an old version of the algorithm, not that >> @@ -41,12 +40,9 @@ def compress(indata, algo, with_header=True): >> data = LZMA_ALONE.compress(indata) >> else: >> raise ValueError("Unknown algorithm '%s'" % algo) >> - if with_header: >> - hdr = struct.pack('<I', len(data)) >> - data = hdr + data >> return data >> >> -def decompress(indata, algo, with_header=True): >> +def decompress(indata, algo): >> """Decompress some data using a given algorithm >> >> Note that for lzma this uses an old version of the algorithm, not that >> @@ -64,9 +60,6 @@ def decompress(indata, algo, with_header=True): >> """ >> if algo == 'none': >> return indata >> - if with_header: >> - data_len = struct.unpack('<I', indata[:4])[0] >> - indata = indata[4:4 + data_len] >> if algo == 'lz4': >> data = LZ4.decompress(indata) >> elif algo == 'lzma': >> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst >> index ae4305c99e..8e722426d3 100644 >> --- a/tools/binman/entries.rst >> +++ b/tools/binman/entries.rst >> @@ -208,6 +208,10 @@ This is a blob containing a device tree. The contents of the blob are >> obtained from the list of available device-tree files, managed by the >> 'state' module. >> >> +Additional Properties / Entry arguments: >> + - prepend: Header type to use: >> + none: No header >> + length: 32-bit length header >> >> >> Entry: blob-ext: Externally built binary blob >> diff --git a/tools/binman/entry.py b/tools/binman/entry.py >> index a07a588864..8cbfd43af9 100644 >> --- a/tools/binman/entry.py >> +++ b/tools/binman/entry.py >> @@ -1069,7 +1069,7 @@ features to produce new behaviours. >> indata: Data to compress >> >> Returns: >> - Compressed data (first word is the compressed size) >> + Compressed data >> """ >> self.uncomp_data = indata >> if self.compress != 'none': >> diff --git a/tools/binman/etype/blob_dtb.py b/tools/binman/etype/blob_dtb.py >> index 4159e3032a..652b8abd8f 100644 >> --- a/tools/binman/etype/blob_dtb.py >> +++ b/tools/binman/etype/blob_dtb.py >> @@ -7,6 +7,8 @@ >> >> from binman.entry import Entry >> from binman.etype.blob import Entry_blob >> +from dtoc import fdt_util >> +import struct >> >> # This is imported if needed >> state = None >> @@ -17,6 +19,9 @@ class Entry_blob_dtb(Entry_blob): >> This is a blob containing a device tree. The contents of the blob are >> obtained from the list of available device-tree files, managed by the >> 'state' module. >> + >> + Additional attributes: >> + prepend: Header used (e.g. 'length'), 'none' if none >> """ >> def __init__(self, section, etype, node): >> # Put this here to allow entry-docs and help to work without libfdt >> @@ -25,6 +30,12 @@ class Entry_blob_dtb(Entry_blob): >> >> super().__init__(section, etype, node) >> >> + self.prepend = 'none' > > None ? I copy this from the compress attribute. You only need one check to support a missing value or a 'none' value. But I don't need this check and can use None. > >> + >> + def ReadNode(self): >> + super().ReadNode() >> + self.prepend = fdt_util.GetString(self._node, 'prepend', 'none') > > Can you drop the 'none' so that it uses None instead? Is 'none' a valid entry? Do we need to distinguish between 'none' and an invalid value? > Aso we should check for a valid value here - e.g. it must be 'length' > and not something else, otherwise self.Raise() Okay. I will remove the 'none' and only support 'length'. Regards Stefan
Hi Stefan, On Mon, 15 Aug 2022 at 01:09, Stefan Herbrechtsmeier <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: > > Hi Simon, > > Am 13.08.2022 um 16:59 schrieb Simon Glass: > > Hi Stefan, > > > > On Mon, 8 Aug 2022 at 04:51, Stefan Herbrechtsmeier [..] > >> # This is imported if needed > >> state = None > >> @@ -17,6 +19,9 @@ class Entry_blob_dtb(Entry_blob): > >> This is a blob containing a device tree. The contents of the blob are > >> obtained from the list of available device-tree files, managed by the > >> 'state' module. > >> + > >> + Additional attributes: > >> + prepend: Header used (e.g. 'length'), 'none' if none > >> """ > >> def __init__(self, section, etype, node): > >> # Put this here to allow entry-docs and help to work without libfdt > >> @@ -25,6 +30,12 @@ class Entry_blob_dtb(Entry_blob): > >> > >> super().__init__(section, etype, node) > >> > >> + self.prepend = 'none' > > > > None ? > > I copy this from the compress attribute. You only need one check to > support a missing value or a 'none' value. But I don't need this check > and can use None. OK I see. The idea there was that people might want to explicitly say 'none'. I;m not sure how use that is, particularly with prepend, but I'm OK with either way. > > > > >> + > >> + def ReadNode(self): > >> + super().ReadNode() > >> + self.prepend = fdt_util.GetString(self._node, 'prepend', 'none') > > > > Can you drop the 'none' so that it uses None instead? > > Is 'none' a valid entry? Do we need to distinguish between 'none' and an > invalid value? Eventually we do...but for now bad things happen. See the TODO in binman for some of that. > > > Aso we should check for a valid value here - e.g. it must be 'length' > > and not something else, otherwise self.Raise() > > Okay. I will remove the 'none' and only support 'length'. As above, up to you. I had forgotten about the compress thing. Regards, Simon
diff --git a/tools/binman/cbfs_util.py b/tools/binman/cbfs_util.py index 9cad03886f..a1836f4ad3 100644 --- a/tools/binman/cbfs_util.py +++ b/tools/binman/cbfs_util.py @@ -241,9 +241,9 @@ class CbfsFile(object): """Handle decompressing data if necessary""" indata = self.data if self.compress == COMPRESS_LZ4: - data = comp_util.decompress(indata, 'lz4', with_header=False) + data = comp_util.decompress(indata, 'lz4') elif self.compress == COMPRESS_LZMA: - data = comp_util.decompress(indata, 'lzma', with_header=False) + data = comp_util.decompress(indata, 'lzma') else: data = indata self.memlen = len(data) @@ -362,9 +362,9 @@ class CbfsFile(object): elif self.ftype == TYPE_RAW: orig_data = data if self.compress == COMPRESS_LZ4: - data = comp_util.compress(orig_data, 'lz4', with_header=False) + data = comp_util.compress(orig_data, 'lz4') elif self.compress == COMPRESS_LZMA: - data = comp_util.compress(orig_data, 'lzma', with_header=False) + data = comp_util.compress(orig_data, 'lzma') self.memlen = len(orig_data) self.data_len = len(data) attr = struct.pack(ATTR_COMPRESSION_FORMAT, diff --git a/tools/binman/comp_util.py b/tools/binman/comp_util.py index dc76adab35..269bbf7975 100644 --- a/tools/binman/comp_util.py +++ b/tools/binman/comp_util.py @@ -3,7 +3,6 @@ # """Utilities to compress and decompress data""" -import struct import tempfile from binman import bintool @@ -16,7 +15,7 @@ LZMA_ALONE = bintool.Bintool.create('lzma_alone') HAVE_LZMA_ALONE = LZMA_ALONE.is_present() -def compress(indata, algo, with_header=True): +def compress(indata, algo): """Compress some data using a given algorithm Note that for lzma this uses an old version of the algorithm, not that @@ -41,12 +40,9 @@ def compress(indata, algo, with_header=True): data = LZMA_ALONE.compress(indata) else: raise ValueError("Unknown algorithm '%s'" % algo) - if with_header: - hdr = struct.pack('<I', len(data)) - data = hdr + data return data -def decompress(indata, algo, with_header=True): +def decompress(indata, algo): """Decompress some data using a given algorithm Note that for lzma this uses an old version of the algorithm, not that @@ -64,9 +60,6 @@ def decompress(indata, algo, with_header=True): """ if algo == 'none': return indata - if with_header: - data_len = struct.unpack('<I', indata[:4])[0] - indata = indata[4:4 + data_len] if algo == 'lz4': data = LZ4.decompress(indata) elif algo == 'lzma': diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst index ae4305c99e..8e722426d3 100644 --- a/tools/binman/entries.rst +++ b/tools/binman/entries.rst @@ -208,6 +208,10 @@ This is a blob containing a device tree. The contents of the blob are obtained from the list of available device-tree files, managed by the 'state' module. +Additional Properties / Entry arguments: + - prepend: Header type to use: + none: No header + length: 32-bit length header Entry: blob-ext: Externally built binary blob diff --git a/tools/binman/entry.py b/tools/binman/entry.py index a07a588864..8cbfd43af9 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -1069,7 +1069,7 @@ features to produce new behaviours. indata: Data to compress Returns: - Compressed data (first word is the compressed size) + Compressed data """ self.uncomp_data = indata if self.compress != 'none': diff --git a/tools/binman/etype/blob_dtb.py b/tools/binman/etype/blob_dtb.py index 4159e3032a..652b8abd8f 100644 --- a/tools/binman/etype/blob_dtb.py +++ b/tools/binman/etype/blob_dtb.py @@ -7,6 +7,8 @@ from binman.entry import Entry from binman.etype.blob import Entry_blob +from dtoc import fdt_util +import struct # This is imported if needed state = None @@ -17,6 +19,9 @@ class Entry_blob_dtb(Entry_blob): This is a blob containing a device tree. The contents of the blob are obtained from the list of available device-tree files, managed by the 'state' module. + + Additional attributes: + prepend: Header used (e.g. 'length'), 'none' if none """ def __init__(self, section, etype, node): # Put this here to allow entry-docs and help to work without libfdt @@ -25,6 +30,12 @@ class Entry_blob_dtb(Entry_blob): super().__init__(section, etype, node) + self.prepend = 'none' + + def ReadNode(self): + super().ReadNode() + self.prepend = fdt_util.GetString(self._node, 'prepend', 'none') + def ObtainContents(self): """Get the device-tree from the list held by the 'state' module""" self._filename = self.GetDefaultFilename() @@ -35,6 +46,9 @@ class Entry_blob_dtb(Entry_blob): """Re-read the DTB contents so that we get any calculated properties""" _, indata = state.GetFdtContents(self.GetFdtEtype()) data = self.CompressData(indata) + if self.prepend == 'length': + hdr = struct.pack('<I', len(data)) + data = hdr + data return self.ProcessContentsUpdate(data) def GetFdtEtype(self): @@ -50,6 +64,13 @@ class Entry_blob_dtb(Entry_blob): fname = self.GetDefaultFilename() return {self.GetFdtEtype(): [self, fname]} + def ReadData(self, decomp=True, alt_format=None): + data = super().ReadData(decomp, alt_format) + if self.prepend == 'length': + data_len = struct.unpack('<I', data[:4])[0] + data = data[4:4 + data_len] + return data + def WriteData(self, data, decomp=True): ok = super().WriteData(data, decomp) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index da9aa9e679..057b4e28b7 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -2536,6 +2536,28 @@ class TestFunctional(unittest.TestCase): } self.assertEqual(expected, props) + def testCompressPrependLengthDtb(self): + """Test that compress of device-tree files with length header is + supported + """ + data = self.data = self._DoReadFileRealDtb('235_compress_prepend_length_dtb.dts') + self.assertEqual(U_BOOT_DATA, data[:len(U_BOOT_DATA)]) + dtb_data = data[len(U_BOOT_DATA):] + comp_data_len = struct.unpack('<I', dtb_data[:4])[0] + comp_data = dtb_data[4:4 + comp_data_len] + 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(dtb_data), + 'size': len(data), + } + self.assertEqual(expected, props) + + def testCbfsUpdateFdt(self): """Test that we can update the device tree with CBFS offset/size info""" self._CheckLz4() @@ -2856,7 +2878,7 @@ class TestFunctional(unittest.TestCase): def testExtractCbfsRaw(self): """Test extracting CBFS compressed data without decompressing it""" data = self._RunExtractCmd('section/cbfs/u-boot-dtb', decomp=False) - dtb = comp_util.decompress(data, 'lzma', with_header=False) + dtb = comp_util.decompress(data, 'lzma') self.assertEqual(EXTRACT_DTB_SIZE, len(dtb)) def testExtractBadEntry(self): @@ -4427,15 +4449,17 @@ class TestFunctional(unittest.TestCase): rest = base[len(U_BOOT_DATA):] # Check compressed data - section1 = self._decompress(rest) expect1 = comp_util.compress(COMPRESS_DATA + U_BOOT_DATA, 'lz4') - self.assertEquals(expect1, rest[:len(expect1)]) + data1 = rest[:len(expect1)] + section1 = self._decompress(data1) + self.assertEquals(expect1, data1) self.assertEquals(COMPRESS_DATA + U_BOOT_DATA, section1) rest1 = rest[len(expect1):] - section2 = self._decompress(rest1) expect2 = comp_util.compress(COMPRESS_DATA + COMPRESS_DATA, 'lz4') - self.assertEquals(expect2, rest1[:len(expect2)]) + data2 = rest1[:len(expect2)] + section2 = self._decompress(data2) + self.assertEquals(expect2, data2) self.assertEquals(COMPRESS_DATA + COMPRESS_DATA, section2) rest2 = rest1[len(expect2):] diff --git a/tools/binman/test/235_compress_prepend_length_dtb.dts b/tools/binman/test/235_compress_prepend_length_dtb.dts new file mode 100644 index 0000000000..a5abc60477 --- /dev/null +++ b/tools/binman/test/235_compress_prepend_length_dtb.dts @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + u-boot { + }; + u-boot-dtb { + compress = "lz4"; + prepend = "length"; + }; + }; +};