| Message ID | 20221217212950.90350-1-sjg@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | binman: Support optional external blobs | expand |
Hi Simon, On 12/17/22 22:29, Simon Glass wrote: > Some blobs are actually not necessary for the board to work correctly. Add > a property to allow this to be indicated. Missing optional blobs do not > cause a build failure. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > tools/binman/binman.rst | 9 +++++++++ > tools/binman/control.py | 11 +++++++++++ > tools/binman/entry.py | 25 +++++++++++++++++++++---- > tools/binman/etype/blob.py | 2 +- > tools/binman/etype/section.py | 14 +++++++++++++- > tools/binman/ftest.py | 10 ++++++++++ > tools/binman/test/266_blob_ext_opt.dts | 21 +++++++++++++++++++++ > 7 files changed, 86 insertions(+), 6 deletions(-) > create mode 100644 tools/binman/test/266_blob_ext_opt.dts > > diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst > index 1ab6d012b83..391494907ab 100644 > --- a/tools/binman/binman.rst > +++ b/tools/binman/binman.rst > @@ -689,6 +689,15 @@ no-expanded: > `no-expanded` property disables this just for a single entry. Put the > `no-expanded` boolean property in the node to select this behaviour. > > +optional: > + External blobs are normally required to be present for the image to be > + built (but see `External blobs`_). This properly allows an entry to be > + optional, so that when it is cannot be found, this problem is ignored and > + an empty file is used for this blob. This should be used only when the blob > + is entirely optional and is not needed for correct operation of the image. Could we provide such an example? Are you thinking about OP-TEE for example? I'm also a bit worried because warnings are notoriously ignored by users if builds do not end up failing. At least binman usually runs at the end of the build, so the warnings wouldn't be hidden in a pile of compiler warnings (thinking about GCC upgrades or vendor u-boot). > + Note that missing, optional blobs do not produce a non-zero exit code from > + binman, although it does show a warning about the missing external blob. > + > The attributes supported for images and sections are described below. Several > are similar to those for entries. > > diff --git a/tools/binman/control.py b/tools/binman/control.py > index 07225381146..e64740094f6 100644 > --- a/tools/binman/control.py > +++ b/tools/binman/control.py > @@ -594,12 +594,14 @@ def ProcessImage(image, update_fdt, write_map, get_contents=True, > image.BuildImage() > if write_map: > image.WriteMap() > + > missing_list = [] > image.CheckMissing(missing_list) > if missing_list: > tout.warning("Image '%s' is missing external blobs and is non-functional: %s" % > (image.name, ' '.join([e.name for e in missing_list]))) > _ShowHelpForMissingBlobs(missing_list) > + > faked_list = [] > image.CheckFakedBlobs(faked_list) > if faked_list: > @@ -607,6 +609,15 @@ def ProcessImage(image, update_fdt, write_map, get_contents=True, > "Image '%s' has faked external blobs and is non-functional: %s" % > (image.name, ' '.join([os.path.basename(e.GetDefaultFilename()) > for e in faked_list]))) > + > + optional_list = [] > + image.CheckOptional(optional_list) > + if optional_list: > + tout.warning( > + "Image '%s' is missing external blobs but is still functional: %s" % > + (image.name, ' '.join([e.name for e in optional_list]))) > + _ShowHelpForMissingBlobs(optional_list) > + > missing_bintool_list = [] > image.check_missing_bintools(missing_bintool_list) > if missing_bintool_list: > diff --git a/tools/binman/entry.py b/tools/binman/entry.py > index de51d295891..d73f3013405 100644 > --- a/tools/binman/entry.py > +++ b/tools/binman/entry.py > @@ -73,7 +73,9 @@ class Entry(object): > 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 > - missing: True if this entry is missing its contents > + missing: True if this entry is missing its contents. Note that if it is > + optional, this entry will not appear in the list generated by > + entry.CheckMissing() since it is considered OK for it to be missing. > allow_missing: Allow children of this entry to be missing (used by > subclasses such as Entry_section) > allow_fake: Allow creating a dummy fake file if the blob file is not > @@ -95,6 +97,7 @@ class Entry(object): > the entry itself, allowing it to vanish in certain circumstances. > An absent entry is removed during processing so that it does not > appear in the map > + optional (bool): True if this entry contains an optional external blob > """ > fake_dir = None > > @@ -138,6 +141,7 @@ class Entry(object): > self.elf_fname = None > self.auto_write_symbols = auto_write_symbols > self.absent = False > + self.optional = False > > @staticmethod > def FindEntryClass(etype, expanded): > @@ -289,6 +293,7 @@ class Entry(object): > self.offset_unset = fdt_util.GetBool(self._node, 'offset-unset') > self.extend_size = fdt_util.GetBool(self._node, 'extend-size') > self.missing_msg = fdt_util.GetString(self._node, 'missing-msg') > + self.optional = fdt_util.GetBool(self._node, 'optional') > > # This is only supported by blobs and sections at present > self.compress = fdt_util.GetString(self._node, 'compress', 'none') > @@ -1039,14 +1044,15 @@ features to produce new behaviours. > self.allow_fake = allow_fake > > def CheckMissing(self, missing_list): > - """Check if any entries in this section have missing external blobs > + """Check if the entry has missing external blobs > > - If there are missing blobs, the entries are added to the list > + If there are missing (non-optional) blobs, the entries are added to the > + list > > Args: > missing_list: List of Entry objects to be added to > """ > - if self.missing: > + if self.missing and not self.optional: > missing_list.append(self) > I'm tempted to have CheckMissing operate on two lists, one optional and one required? I'm not sure there's a need to do separate calls for optional and required missing blobs and it would make it harder to not forget about checking those in the right places. > def check_fake_fname(self, fname, size=0): > @@ -1085,6 +1091,17 @@ features to produce new behaviours. > # This is meaningless for anything other than blobs > pass > > + def CheckOptional(self, optional_list): If still going for separate functions, I would at least name this one CheckOptionalMissing to make it a bit more obvious what we're after without reading the function documentation. Cheers, Quentin
On Mon, Jan 02, 2023 at 11:52:17AM +0100, Quentin Schulz wrote: > Hi Simon, > > On 12/17/22 22:29, Simon Glass wrote: > > Some blobs are actually not necessary for the board to work correctly. Add > > a property to allow this to be indicated. Missing optional blobs do not > > cause a build failure. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > tools/binman/binman.rst | 9 +++++++++ > > tools/binman/control.py | 11 +++++++++++ > > tools/binman/entry.py | 25 +++++++++++++++++++++---- > > tools/binman/etype/blob.py | 2 +- > > tools/binman/etype/section.py | 14 +++++++++++++- > > tools/binman/ftest.py | 10 ++++++++++ > > tools/binman/test/266_blob_ext_opt.dts | 21 +++++++++++++++++++++ > > 7 files changed, 86 insertions(+), 6 deletions(-) > > create mode 100644 tools/binman/test/266_blob_ext_opt.dts > > > > diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst > > index 1ab6d012b83..391494907ab 100644 > > --- a/tools/binman/binman.rst > > +++ b/tools/binman/binman.rst > > @@ -689,6 +689,15 @@ no-expanded: > > `no-expanded` property disables this just for a single entry. Put the > > `no-expanded` boolean property in the node to select this behaviour. > > +optional: > > + External blobs are normally required to be present for the image to be > > + built (but see `External blobs`_). This properly allows an entry to be > > + optional, so that when it is cannot be found, this problem is ignored and > > + an empty file is used for this blob. This should be used only when the blob > > + is entirely optional and is not needed for correct operation of the image. > > Could we provide such an example? The example is the pmic firmware (SCP) for sunxi platforms. That can be omitted safely (which results in some PM features not working).
diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst index 1ab6d012b83..391494907ab 100644 --- a/tools/binman/binman.rst +++ b/tools/binman/binman.rst @@ -689,6 +689,15 @@ no-expanded: `no-expanded` property disables this just for a single entry. Put the `no-expanded` boolean property in the node to select this behaviour. +optional: + External blobs are normally required to be present for the image to be + built (but see `External blobs`_). This properly allows an entry to be + optional, so that when it is cannot be found, this problem is ignored and + an empty file is used for this blob. This should be used only when the blob + is entirely optional and is not needed for correct operation of the image. + Note that missing, optional blobs do not produce a non-zero exit code from + binman, although it does show a warning about the missing external blob. + The attributes supported for images and sections are described below. Several are similar to those for entries. diff --git a/tools/binman/control.py b/tools/binman/control.py index 07225381146..e64740094f6 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -594,12 +594,14 @@ def ProcessImage(image, update_fdt, write_map, get_contents=True, image.BuildImage() if write_map: image.WriteMap() + missing_list = [] image.CheckMissing(missing_list) if missing_list: tout.warning("Image '%s' is missing external blobs and is non-functional: %s" % (image.name, ' '.join([e.name for e in missing_list]))) _ShowHelpForMissingBlobs(missing_list) + faked_list = [] image.CheckFakedBlobs(faked_list) if faked_list: @@ -607,6 +609,15 @@ def ProcessImage(image, update_fdt, write_map, get_contents=True, "Image '%s' has faked external blobs and is non-functional: %s" % (image.name, ' '.join([os.path.basename(e.GetDefaultFilename()) for e in faked_list]))) + + optional_list = [] + image.CheckOptional(optional_list) + if optional_list: + tout.warning( + "Image '%s' is missing external blobs but is still functional: %s" % + (image.name, ' '.join([e.name for e in optional_list]))) + _ShowHelpForMissingBlobs(optional_list) + missing_bintool_list = [] image.check_missing_bintools(missing_bintool_list) if missing_bintool_list: diff --git a/tools/binman/entry.py b/tools/binman/entry.py index de51d295891..d73f3013405 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -73,7 +73,9 @@ class Entry(object): 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 - missing: True if this entry is missing its contents + missing: True if this entry is missing its contents. Note that if it is + optional, this entry will not appear in the list generated by + entry.CheckMissing() since it is considered OK for it to be missing. allow_missing: Allow children of this entry to be missing (used by subclasses such as Entry_section) allow_fake: Allow creating a dummy fake file if the blob file is not @@ -95,6 +97,7 @@ class Entry(object): the entry itself, allowing it to vanish in certain circumstances. An absent entry is removed during processing so that it does not appear in the map + optional (bool): True if this entry contains an optional external blob """ fake_dir = None @@ -138,6 +141,7 @@ class Entry(object): self.elf_fname = None self.auto_write_symbols = auto_write_symbols self.absent = False + self.optional = False @staticmethod def FindEntryClass(etype, expanded): @@ -289,6 +293,7 @@ class Entry(object): self.offset_unset = fdt_util.GetBool(self._node, 'offset-unset') self.extend_size = fdt_util.GetBool(self._node, 'extend-size') self.missing_msg = fdt_util.GetString(self._node, 'missing-msg') + self.optional = fdt_util.GetBool(self._node, 'optional') # This is only supported by blobs and sections at present self.compress = fdt_util.GetString(self._node, 'compress', 'none') @@ -1039,14 +1044,15 @@ features to produce new behaviours. self.allow_fake = allow_fake def CheckMissing(self, missing_list): - """Check if any entries in this section have missing external blobs + """Check if the entry has missing external blobs - If there are missing blobs, the entries are added to the list + If there are missing (non-optional) blobs, the entries are added to the + list Args: missing_list: List of Entry objects to be added to """ - if self.missing: + if self.missing and not self.optional: missing_list.append(self) def check_fake_fname(self, fname, size=0): @@ -1085,6 +1091,17 @@ features to produce new behaviours. # This is meaningless for anything other than blobs pass + def CheckOptional(self, optional_list): + """Check if the entry has missing but optional external blobs + + If there are missing (optional) blobs, the entries are added to the list + + Args: + optional_list (list): List of Entry objects to be added to + """ + if self.missing and self.optional: + optional_list.append(self) + def GetAllowMissing(self): """Get whether a section allows missing external blobs diff --git a/tools/binman/etype/blob.py b/tools/binman/etype/blob.py index a50a8068901..70dea7158ee 100644 --- a/tools/binman/etype/blob.py +++ b/tools/binman/etype/blob.py @@ -39,7 +39,7 @@ class Entry_blob(Entry): def ObtainContents(self, fake_size=0): self._filename = self.GetDefaultFilename() self._pathname = tools.get_input_filename(self._filename, - self.external and self.section.GetAllowMissing()) + self.external and (self.optional or self.section.GetAllowMissing())) # Allow the file to be missing if not self._pathname: self._pathname, faked = self.check_fake_fname(self._filename, diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 57bfee0b286..44dafaf7262 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -863,7 +863,8 @@ class Entry_section(Entry): def CheckMissing(self, missing_list): """Check if any entries in this section have missing external blobs - If there are missing blobs, the entries are added to the list + If there are missing (non-optional) blobs, the entries are added to the + list Args: missing_list: List of Entry objects to be added to @@ -882,6 +883,17 @@ class Entry_section(Entry): for entry in self._entries.values(): entry.CheckFakedBlobs(faked_blobs_list) + def CheckOptional(self, optional_list): + """Check the section for missing but optional external blobs + + If there are missing (optional) blobs, the entries are added to the list + + Args: + optional_list (list): List of Entry objects to be added to + """ + for entry in self._entries.values(): + entry.CheckOptional(optional_list) + def check_missing_bintools(self, missing_list): """Check if any entries in this section have missing bintools diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index f893050e706..330e8e1ccb4 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -6178,6 +6178,16 @@ fdt fdtmap Extract the devicetree blob from the fdtmap "Node '/binman/fit/images/@tee-SEQ/tee-os': Invalid OP-TEE file: size mismatch (expected 0x4, have 0xe)", str(exc.exception)) + def testExtblobOptional(self): + """Test an image with an external blob that is optional""" + with test_util.capture_sys_output() as (stdout, stderr): + data = self._DoReadFile('266_blob_ext_opt.dts') + self.assertEqual(REFCODE_DATA, data) + err = stderr.getvalue() + self.assertRegex( + err, + "Image '.*' is missing external blobs but is still functional: missing") + if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/266_blob_ext_opt.dts b/tools/binman/test/266_blob_ext_opt.dts new file mode 100644 index 00000000000..717153152ce --- /dev/null +++ b/tools/binman/test/266_blob_ext_opt.dts @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + ok { + type = "blob-ext"; + filename = "refcode.bin"; + }; + + missing { + type = "blob-ext"; + filename = "missing.bin"; + optional; + }; + }; +};
Some blobs are actually not necessary for the board to work correctly. Add a property to allow this to be indicated. Missing optional blobs do not cause a build failure. Signed-off-by: Simon Glass <sjg@chromium.org> --- tools/binman/binman.rst | 9 +++++++++ tools/binman/control.py | 11 +++++++++++ tools/binman/entry.py | 25 +++++++++++++++++++++---- tools/binman/etype/blob.py | 2 +- tools/binman/etype/section.py | 14 +++++++++++++- tools/binman/ftest.py | 10 ++++++++++ tools/binman/test/266_blob_ext_opt.dts | 21 +++++++++++++++++++++ 7 files changed, 86 insertions(+), 6 deletions(-) create mode 100644 tools/binman/test/266_blob_ext_opt.dts