From bf984a6a0073f9a1bf7814ac44e72218afcb2081 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 23 Jun 2024 11:55:14 -0600 Subject: [PATCH 01/17] buildman: Retry the build for current source Buildman retries a failed build when processing a branch, but does not do this when building current source. It is useful to do this retry in both cases, so add the logic for it. Signed-off-by: Simon Glass --- tools/buildman/builderthread.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py index bbe2f6f0d24..55658487abf 100644 --- a/tools/buildman/builderthread.py +++ b/tools/buildman/builderthread.py @@ -755,6 +755,14 @@ class BuilderThread(threading.Thread): self.mrproper, self.builder.config_only, True, self.builder.force_build_failures, job.work_in_output, job.adjust_cfg) + failed = result.return_code or result.stderr + if failed and not self.mrproper: + result, request_config = self.run_commit(None, brd, work_dir, + True, self.builder.fallback_mrproper, + self.builder.config_only, True, + self.builder.force_build_failures, + job.work_in_output, job.adjust_cfg) + result.commit_upto = 0 self._write_result(result, job.keep_outputs, job.work_in_output) self._send_result(result) From ba134c35316e275fd8708cf20794f61526d6420a Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 15 Aug 2024 13:57:45 -0600 Subject: [PATCH 02/17] buildman: Allow skipping the dtc build For most boards, the device-tree compiler is built in-tree, ignoring the system version. Add a special option to skip this build. This can be useful when the system dtc is up-to-date, as it speeds up the build. Signed-off-by: Simon Glass --- tools/buildman/builder.py | 27 ++++++++++++++++++++++++++- tools/buildman/builderthread.py | 4 ++-- tools/buildman/buildman.rst | 3 +++ tools/buildman/cmdline.py | 2 ++ tools/buildman/control.py | 3 ++- tools/buildman/test.py | 31 +++++++++++++++++++++++++++++++ 6 files changed, 66 insertions(+), 4 deletions(-) diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py index c4384f53e8d..4090d328b30 100644 --- a/tools/buildman/builder.py +++ b/tools/buildman/builder.py @@ -22,6 +22,7 @@ from buildman import toolchain from patman import gitutil from u_boot_pylib import command from u_boot_pylib import terminal +from u_boot_pylib import tools from u_boot_pylib.terminal import tprint # This indicates an new int or hex Kconfig property with no default @@ -263,7 +264,8 @@ class Builder: adjust_cfg=None, allow_missing=False, no_lto=False, reproducible_builds=False, force_build=False, force_build_failures=False, force_reconfig=False, - in_tree=False, force_config_on_failure=False, make_func=None): + in_tree=False, force_config_on_failure=False, make_func=None, + dtc_skip=False): """Create a new Builder object Args: @@ -312,6 +314,7 @@ class Builder: force_config_on_failure (bool): Reconfigure the build before retrying a failed build make_func (function): Function to call to run 'make' + dtc_skip (bool): True to skip building dtc and use the system one """ self.toolchains = toolchains self.base_dir = base_dir @@ -354,6 +357,12 @@ class Builder: self.in_tree = in_tree self.force_config_on_failure = force_config_on_failure self.fallback_mrproper = fallback_mrproper + if dtc_skip: + self.dtc = shutil.which('dtc') + if not self.dtc: + raise ValueError('Cannot find dtc') + else: + self.dtc = None if not self.squash_config_y: self.config_filenames += EXTRA_CONFIG_FILENAMES @@ -407,6 +416,22 @@ class Builder: def signal_handler(self, signal, frame): sys.exit(1) + def make_environment(self, toolchain): + """Create the environment to use for building + + Args: + toolchain (Toolchain): Toolchain to use for building + + Returns: + dict: + key (str): Variable name + value (str): Variable value + """ + env = toolchain.MakeEnvironment(self.full_path) + if self.dtc: + env[b'DTC'] = tools.to_bytes(self.dtc) + return env + def set_display_options(self, show_errors=False, show_sizes=False, show_detail=False, show_bloat=False, list_error_boards=False, show_config=False, diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py index 55658487abf..b5afee61aff 100644 --- a/tools/buildman/builderthread.py +++ b/tools/buildman/builderthread.py @@ -406,7 +406,7 @@ class BuilderThread(threading.Thread): the next incremental build """ # Set up the environment and command line - env = self.toolchain.MakeEnvironment(self.builder.full_path) + env = self.builder.make_environment(self.toolchain) mkdir(out_dir) args, cwd, src_dir = self._build_args(brd, out_dir, out_rel_dir, @@ -574,7 +574,7 @@ class BuilderThread(threading.Thread): outf.write(f'{result.return_code}') # Write out the image and function size information and an objdump - env = result.toolchain.MakeEnvironment(self.builder.full_path) + env = self.builder.make_environment(self.toolchain) with open(os.path.join(build_dir, 'out-env'), 'wb') as outf: for var in sorted(env.keys()): outf.write(b'%s="%s"' % (var, env[var])) diff --git a/tools/buildman/buildman.rst b/tools/buildman/buildman.rst index b8ff3bf1ab2..e873611e596 100644 --- a/tools/buildman/buildman.rst +++ b/tools/buildman/buildman.rst @@ -1030,6 +1030,9 @@ of the source tree, thus allowing rapid tested evolution of the code:: ./tools/buildman/buildman -Pr tegra +Note also the `--dtc-skip` option which uses the system device-tree compiler to +avoid needing to build it for each board. This can save 10-20% of build time. +An alternative is to set DTC=/path/to/dtc when running buildman. Checking configuration ---------------------- diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py index 544a391a464..7573e5bdfe8 100644 --- a/tools/buildman/cmdline.py +++ b/tools/buildman/cmdline.py @@ -46,6 +46,8 @@ def add_upto_m(parser): help='Show detailed size delta for each board in the -S summary') parser.add_argument('-D', '--debug', action='store_true', help='Enabling debugging (provides a full traceback on error)') + parser.add_argument('--dtc-skip', action='store_true', default=False, + help='Skip building of dtc and use the system version') parser.add_argument('-e', '--show_errors', action='store_true', default=False, help='Show errors and warnings') parser.add_argument('-E', '--warnings-as-errors', action='store_true', diff --git a/tools/buildman/control.py b/tools/buildman/control.py index d3d027f02ab..55d4d770c5c 100644 --- a/tools/buildman/control.py +++ b/tools/buildman/control.py @@ -809,7 +809,8 @@ def do_buildman(args, toolchains=None, make_func=None, brds=None, force_build = args.force_build, force_build_failures = args.force_build_failures, force_reconfig = args.force_reconfig, in_tree = args.in_tree, - force_config_on_failure=not args.quick, make_func=make_func) + force_config_on_failure=not args.quick, make_func=make_func, + dtc_skip=args.dtc_skip) TEST_BUILDER = builder diff --git a/tools/buildman/test.py b/tools/buildman/test.py index 46aa2a17916..15801f6097f 100644 --- a/tools/buildman/test.py +++ b/tools/buildman/test.py @@ -999,6 +999,37 @@ class TestBuild(unittest.TestCase): self.assertEqual( {b'CROSS_COMPILE': b'fred aarch64-linux-', b'LC_ALL': b'C'}, diff) + def test_skip_dtc(self): + """Test skipping building the dtc tool""" + old_path = os.getenv('PATH') + try: + os.environ['PATH'] = self.base_dir + + # Check a missing tool + with self.assertRaises(ValueError) as exc: + builder.Builder(self.toolchains, self.base_dir, None, 0, 2, + dtc_skip=True) + self.assertIn('Cannot find dtc', str(exc.exception)) + + # Create a fake tool to use + dtc = os.path.join(self.base_dir, 'dtc') + tools.write_file(dtc, b'xx') + os.chmod(dtc, 0o777) + + build = builder.Builder(self.toolchains, self.base_dir, None, 0, 2, + dtc_skip=True) + toolchain = self.toolchains.Select('arm') + env = build.make_environment(toolchain) + self.assertIn(b'DTC', env) + + # Try the normal case, i.e. not skipping the dtc build + build = builder.Builder(self.toolchains, self.base_dir, None, 0, 2) + toolchain = self.toolchains.Select('arm') + env = build.make_environment(toolchain) + self.assertNotIn(b'DTC', env) + finally: + os.environ['PATH'] = old_path + if __name__ == "__main__": unittest.main() From 7d77ad906146f2fb0811e5780a0109679e7556b5 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Aug 2024 13:11:29 -0600 Subject: [PATCH 03/17] binman: Fix up test coverage for mkeficapsule Add tests for missing tools to complete the test coverage for this etype. Signed-off-by: Simon Glass --- tools/binman/etype/efi_capsule.py | 2 ++ tools/binman/ftest.py | 22 +++++++++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/tools/binman/etype/efi_capsule.py b/tools/binman/etype/efi_capsule.py index 768e006dc50..9f06cc88e6e 100644 --- a/tools/binman/etype/efi_capsule.py +++ b/tools/binman/etype/efi_capsule.py @@ -151,6 +151,8 @@ class Entry_efi_capsule(Entry_section): return tools.read_file(capsule_fname) else: # Bintool is missing; just use the input data as the output + if not self.GetAllowMissing(): + self.Raise("Missing tool: 'mkeficapsule'") self.record_missing_bintool(self.mkeficapsule) return data diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 2577c0016c0..6f515960c85 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -403,8 +403,10 @@ class TestFunctional(unittest.TestCase): test_section_timeout: True to force the first time to timeout, as used in testThreadTimeout() update_fdt_in_elf: Value to pass with --update-fdt-in-elf=xxx - force_missing_tools (str): comma-separated list of bintools to + force_missing_bintools (str): comma-separated list of bintools to regard as missing + ignore_missing (bool): True to return success even if there are + missing blobs or bintools output_dir: Specific output directory to use for image using -O Returns: @@ -7690,6 +7692,24 @@ fdt fdtmap Extract the devicetree blob from the fdtmap # Make sure the other node is gone self.assertIsNone(dtb.GetNode('/node/other-node')) + def testMkeficapsuleMissing(self): + """Test that binman complains if mkeficapsule is missing""" + with self.assertRaises(ValueError) as e: + self._DoTestFile('311_capsule.dts', + force_missing_bintools='mkeficapsule') + self.assertIn("Node '/binman/efi-capsule': Missing tool: 'mkeficapsule'", + str(e.exception)) + + def testMkeficapsuleMissingOk(self): + """Test that binman deals with mkeficapsule being missing""" + with test_util.capture_sys_output() as (stdout, stderr): + ret = self._DoTestFile('311_capsule.dts', + force_missing_bintools='mkeficapsule', + allow_missing=True) + self.assertEqual(103, ret) + err = stderr.getvalue() + self.assertRegex(err, "Image 'image'.*missing bintools.*: mkeficapsule") + if __name__ == "__main__": unittest.main() From 548e86198bd0bf659118bd8f9060c927b77e6e6e Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Aug 2024 13:11:30 -0600 Subject: [PATCH 04/17] binman: Correct the comment for fdtgrep This returns stdout, not a CommandResult so update the comment. Signed-off-by: Simon Glass --- tools/binman/btool/fdtgrep.py | 3 +-- tools/binman/etype/fit.py | 3 +++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/tools/binman/btool/fdtgrep.py b/tools/binman/btool/fdtgrep.py index da1f8c7bf4e..446b2f4144b 100644 --- a/tools/binman/btool/fdtgrep.py +++ b/tools/binman/btool/fdtgrep.py @@ -74,8 +74,7 @@ class Bintoolfdtgrep(bintool.Bintool): (with only neceesary nodes and properties) Returns: - CommandResult: Resulting output from the bintool, or None if the - tool is not present + str or bytes: Resulting stdout from the bintool """ if phase == 'tpl': tag = 'bootph-pre-sram' diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index ee44e5a1cd6..b957df54548 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -550,6 +550,9 @@ class Entry_fit(Entry_section): phase (str): Phase to generate for ('tpl', 'vpl', 'spl') outfile (str): Output filename to write the grepped FDT contents to (with only neceesary nodes and properties) + + Returns: + str or bytes: Resulting stdout from fdtgrep """ return self.fdtgrep.create_for_phase(infile, phase, outfile, self._remove_props) From be45bb941abe18aea0ae03af3d4ce8797885916f Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Aug 2024 13:11:31 -0600 Subject: [PATCH 05/17] binman: Tidy up comments for Entry.GetEntryArgsOrProps() Improve the comments for this function. Signed-off-by: Simon Glass --- tools/binman/entry.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 6d2f3789940..7d4d4692776 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -576,8 +576,16 @@ class Entry(object): def GetEntryArgsOrProps(self, props, required=False): """Return the values of a set of properties + Looks up the named entryargs and returns the value for each. If any + required ones are missing, the error is reported to the user. + Args: - props: List of EntryArg objects + props (list of EntryArg): List of entry arguments to look up + required (bool): True if these entry arguments are required + + Returns: + list of values: one for each item in props, the type is determined + by the EntryArg's 'datatype' property (str or int) Raises: ValueError if a property is not found From fb428a63c189a3934ff1ffc9d2d3b9e05988a59d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Aug 2024 13:11:32 -0600 Subject: [PATCH 06/17] binman: Tidy up comments and pylint warnings in fit Update this entry type to resolve some pylint warnings and make sure that functions and members are fully commented. Signed-off-by: Simon Glass --- tools/binman/etype/fit.py | 73 ++++++++++++++++++++++----------------- 1 file changed, 42 insertions(+), 31 deletions(-) diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index b957df54548..f25226d3a73 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -6,9 +6,10 @@ """Entry-type module for producing a FIT""" import glob -import libfdt import os +import libfdt + from binman.entry import Entry, EntryArg from binman.etype.section import Entry_section from binman import elf @@ -23,6 +24,7 @@ OPERATIONS = { 'split-elf': OP_SPLIT_ELF, } +# pylint: disable=invalid-name class Entry_fit(Entry_section): """Flat Image Tree (FIT) @@ -381,31 +383,46 @@ class Entry_fit(Entry_section): def __init__(self, section, etype, node): """ Members: - _fit: FIT file being built - _entries: dict from Entry_section: + _fit (str): FIT file being built + _fit_props (list of str): 'fit,...' properties found in the + top-level node + _fdts (list of str): Filenames of .dtb files to process + _fdt_dir (str): Directory to scan to find .dtb files, or None + _fit_list_prop (str): Name of the EntryArg containing a list of .dtb + files + _fit_default_dt (str): Name of the EntryArg containing the default + .dtb file + _entries (dict of entries): from Entry_section: key: relative path to entry Node (from the base of the FIT) value: Entry_section object comprising the contents of this node - _priv_entries: Internal copy of _entries which includes 'generator' - entries which are used to create the FIT, but should not be - processed as real entries. This is set up once we have the - entries - _loadables: List of generated split-elf nodes, each a node name + _priv_entries (dict of entries): Internal copy of _entries which + includes 'generator' entries which are used to create the FIT, + but should not be processed as real entries. This is set up once + we have the entries + _loadables (list of str): List of generated split-elf nodes, each + a node name + _remove_props (list of str): Value of of-spl-remove-props EntryArg, + the list of properties to remove with fdtgrep + mkimage (Bintool): mkimage tool + fdtgrep (Bintool): fdtgrep tool """ super().__init__(section, etype, node) self._fit = None self._fit_props = {} self._fdts = None self._fdt_dir = None - self.mkimage = None - self.fdtgrep = None + self._fit_list_prop = None + self._fit_default_dt = None self._priv_entries = {} self._loadables = [] self._remove_props = [] - props, = self.GetEntryArgsOrProps( - [EntryArg('of-spl-remove-props', str)], required=False) + props = self.GetEntryArgsOrProps( + [EntryArg('of-spl-remove-props', str)], required=False)[0] if props: self._remove_props = props.split() + self.mkimage = None + self.fdtgrep = None def ReadNode(self): super().ReadNode() @@ -414,8 +431,8 @@ class Entry_fit(Entry_section): self._fit_props[pname] = prop self._fit_list_prop = self._fit_props.get('fit,fdt-list') if self._fit_list_prop: - fdts, = self.GetEntryArgsOrProps( - [EntryArg(self._fit_list_prop.value, str)]) + fdts = self.GetEntryArgsOrProps( + [EntryArg(self._fit_list_prop.value, str)])[0] if fdts is not None: self._fdts = fdts.split() else: @@ -431,7 +448,7 @@ class Entry_fit(Entry_section): self._fit_default_dt = self.GetEntryArgsOrProps([EntryArg('default-dt', str)])[0] - def _get_operation(self, base_node, node): + def _get_operation(self, node): """Get the operation referenced by a subnode Args: @@ -560,9 +577,6 @@ class Entry_fit(Entry_section): def _build_input(self): """Finish the FIT by adding the 'data' properties to it - Arguments: - fdt: FIT to update - Returns: bytes: New fdt contents """ @@ -637,7 +651,7 @@ class Entry_fit(Entry_section): result.append(name) return firmware, result - def _gen_fdt_nodes(base_node, node, depth, in_images): + def _gen_fdt_nodes(node, depth, in_images): """Generate FDT nodes This creates one node for each member of self._fdts using the @@ -710,11 +724,10 @@ class Entry_fit(Entry_section): else: self.Raise("Generator node requires 'fit,fdt-list' property") - def _gen_split_elf(base_node, node, depth, segments, entry_addr): + def _gen_split_elf(node, depth, segments, entry_addr): """Add nodes for the ELF file, one per group of contiguous segments Args: - base_node (Node): Template node from the binman definition node (Node): Node to replace (in the FIT being built) depth: Current node depth (0 is the base 'fit' node) segments (list): list of segments, each: @@ -745,7 +758,7 @@ class Entry_fit(Entry_section): with fsw.add_node(subnode.name): _add_node(node, depth + 1, subnode) - def _gen_node(base_node, node, depth, in_images, entry): + def _gen_node(node, depth, in_images, entry): """Generate nodes from a template This creates one or more nodes depending on the fit,operation being @@ -761,8 +774,6 @@ class Entry_fit(Entry_section): If the file is missing, nothing is generated. Args: - base_node (Node): Base Node of the FIT (with 'description' - property) node (Node): Generator node to process depth (int): Current node depth (0 is the base 'fit' node) in_images (bool): True if this is inside the 'images' node, so @@ -770,13 +781,12 @@ class Entry_fit(Entry_section): entry (entry_Section): Entry for the section containing the contents of this node """ - oper = self._get_operation(base_node, node) + oper = self._get_operation(node) if oper == OP_GEN_FDT_NODES: - _gen_fdt_nodes(base_node, node, depth, in_images) + _gen_fdt_nodes(node, depth, in_images) elif oper == OP_SPLIT_ELF: # Entry_section.ObtainContents() either returns True or # raises an exception. - data = None missing_opt_list = [] entry.ObtainContents() entry.Pack(0) @@ -798,7 +808,7 @@ class Entry_fit(Entry_section): self._raise_subnode( node, f'Failed to read ELF file: {str(exc)}') - _gen_split_elf(base_node, node, depth, segments, entry_addr) + _gen_split_elf(node, depth, segments, entry_addr) def _add_node(base_node, depth, node): """Add nodes to the output FIT @@ -829,7 +839,6 @@ class Entry_fit(Entry_section): fsw.property('data', bytes(data)) for subnode in node.subnodes: - subnode_path = f'{rel_path}/{subnode.name}' if has_images and not self.IsSpecialSubnode(subnode): # This subnode is a content node not meant to appear in # the FIT (e.g. "/images/kernel/u-boot"), so don't call @@ -837,7 +846,7 @@ class Entry_fit(Entry_section): pass elif self.GetImage().generate and subnode.name.startswith('@'): entry = self._priv_entries.get(subnode.name) - _gen_node(base_node, subnode, depth, in_images, entry) + _gen_node(subnode, depth, in_images, entry) # This is a generator (template) entry, so remove it from # the list of entries used by PackEntries(), etc. Otherwise # it will appear in the binman output @@ -917,6 +926,8 @@ class Entry_fit(Entry_section): # This should never happen else: # pragma: no cover + offset = None + size = None self.Raise(f'{path}: missing data properties') section.SetOffsetSize(offset, size) @@ -950,7 +961,7 @@ class Entry_fit(Entry_section): if input_fname: fname = input_fname else: - fname = tools.get_output_filename('%s.fit' % uniq) + fname = tools.get_output_filename(f'{uniq}.fit') tools.write_file(fname, self.GetData()) args.append(fname) From 8498d550c59090f73ca4cc60c17a87dd2350f47e Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Aug 2024 13:11:33 -0600 Subject: [PATCH 07/17] binman: Avoid setting the image_pos attribute directly Two places set this attribute directly. Update them to use the function provided. Signed-off-by: Simon Glass --- tools/binman/etype/atf_fip.py | 2 +- tools/binman/etype/cbfs.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/binman/etype/atf_fip.py b/tools/binman/etype/atf_fip.py index 3da0dfcfc12..636e073afc8 100644 --- a/tools/binman/etype/atf_fip.py +++ b/tools/binman/etype/atf_fip.py @@ -248,7 +248,7 @@ class Entry_atf_fip(Entry_section): fent = entry._fip_entry entry.size = fent.size entry.offset = fent.offset - entry.image_pos = self.image_pos + entry.offset + entry.SetImagePos(image_pos + self.offset) def ReadChildData(self, child, decomp=True, alt_format=None): if not self.reader: diff --git a/tools/binman/etype/cbfs.py b/tools/binman/etype/cbfs.py index 575aa624f6c..124fa1e4ffc 100644 --- a/tools/binman/etype/cbfs.py +++ b/tools/binman/etype/cbfs.py @@ -245,7 +245,7 @@ class Entry_cbfs(Entry): cfile = entry._cbfs_file entry.size = cfile.data_len entry.offset = cfile.calced_cbfs_offset - entry.image_pos = self.image_pos + entry.offset + entry.SetImagePos(image_pos + self.offset) if entry._cbfs_compress: entry.uncomp_size = cfile.memlen From 52983ff54b3eca3485dcd54060bdb65423dac7ac Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Aug 2024 13:11:34 -0600 Subject: [PATCH 08/17] binman: Update fdt-list-dir to use the provided directory Since the files are known to be in the provided directory, use that instead of requiring it to be added to the list of input directories. Signed-off-by: Simon Glass --- tools/binman/etype/fit.py | 10 ++++++++-- tools/binman/ftest.py | 7 ++++++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index f25226d3a73..51c82c55e4a 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -96,7 +96,10 @@ class Entry_fit(Entry_section): can be provided as a directory. Each .dtb file in the directory is processed, , e.g.:: - fit,fdt-list-dir = "arch/arm/dts + fit,fdt-list-dir = "arch/arm/dts"; + + In this case the input directories are ignored and all devicetree + files must be in that directory. Substitutions ~~~~~~~~~~~~~ @@ -671,7 +674,10 @@ class Entry_fit(Entry_section): # Generate nodes for each FDT for seq, fdt_fname in enumerate(self._fdts): node_name = node.name[1:].replace('SEQ', str(seq + 1)) - fname = tools.get_input_filename(fdt_fname + '.dtb') + if self._fdt_dir: + fname = os.path.join(self._fdt_dir, fdt_fname + '.dtb') + else: + fname = tools.get_input_filename(fdt_fname + '.dtb') fdt_phase = None with fsw.add_node(node_name): for pname, prop in node.props.items(): diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 6f515960c85..3f05559501d 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -7626,7 +7626,12 @@ fdt fdtmap Extract the devicetree blob from the fdtmap def testFitFdtListDir(self): """Test an image with an FIT with FDT images using fit,fdt-list-dir""" - self.CheckFitFdt('333_fit_fdt_dir.dts', False) + old_dir = os.getcwd() + try: + os.chdir(self._indir) + self.CheckFitFdt('333_fit_fdt_dir.dts', False) + finally: + os.chdir(old_dir) def testFitFdtCompat(self): """Test an image with an FIT with compatible in the config nodes""" From a1fe67a58c6ef90edc08906b4f785e0f6ff45c0e Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Aug 2024 13:11:35 -0600 Subject: [PATCH 09/17] binman: fit: Avoid assuming that a FIT member is a section Use the more generic variable name 'entry' to avoid confusion. Signed-off-by: Simon Glass --- tools/binman/etype/fit.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 51c82c55e4a..3adc4a959e8 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -904,7 +904,7 @@ class Entry_fit(Entry_section): fdt = Fdt.FromData(self.GetData()) fdt.Scan() - for image_name, section in self._entries.items(): + for image_name, entry in self._entries.items(): path = f"/images/{image_name}" node = fdt.GetNode(path) @@ -936,8 +936,8 @@ class Entry_fit(Entry_section): size = None self.Raise(f'{path}: missing data properties') - section.SetOffsetSize(offset, size) - section.SetImagePos(self.image_pos) + entry.SetOffsetSize(offset, size) + entry.SetImagePos(image_pos + self.offset) def AddBintools(self, btools): super().AddBintools(btools) From 6fac8e4883d59e78b7195bf75c174544a4ccfa2d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Aug 2024 13:11:36 -0600 Subject: [PATCH 10/17] binman: fit: Set the image_pos attributes only once The section etype has its own implementation of SetImagePos(), most of which is not useful since the code is included here. So call Entry.SetImagePos() which has the only piece of this which we actually want. Signed-off-by: Simon Glass --- tools/binman/etype/fit.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 3adc4a959e8..96f4fdf3333 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -894,7 +894,10 @@ class Entry_fit(Entry_section): """ if self.build_done: return - super().SetImagePos(image_pos) + + # Skip the section processing, since we do that below. Just call the + # entry method + Entry.SetImagePos(self, image_pos) # If mkimage is missing we'll have empty data, # which will cause a FDT_ERR_BADMAGIC error From c8b7d72b43c649070d29228899277ac3b39ccf29 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Aug 2024 13:11:37 -0600 Subject: [PATCH 11/17] binman: fit: Refine handling of devicetrees for OF_UPSTREAM With OF_UPSTREAM the dts files are in an SoC-specific subdirectory, meaning that the resulting dtb files all end up in a similar subdirectory. We don't want the subdirectory name to appear as a node name in the FIT, so handle this as a special case. Also the default devicetree may have a directory-name prefix, so handle that when searching through the available devicetree files. Signed-off-by: Simon Glass --- tools/binman/etype/fit.py | 19 ++++++++++++------- tools/binman/ftest.py | 15 ++++++++++++++- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 96f4fdf3333..0abe1c78c43 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -600,13 +600,17 @@ class Entry_fit(Entry_section): if val.startswith('@'): if not self._fdts: return - if not self._fit_default_dt: + default_dt = self._fit_default_dt + if not default_dt: self.Raise("Generated 'default' node requires default-dt entry argument") - if self._fit_default_dt not in self._fdts: - self.Raise( - f"default-dt entry argument '{self._fit_default_dt}' " - f"not found in fdt list: {', '.join(self._fdts)}") - seq = self._fdts.index(self._fit_default_dt) + if default_dt not in self._fdts: + if self._fdt_dir: + default_dt = os.path.basename(default_dt) + if default_dt not in self._fdts: + self.Raise( + f"default-dt entry argument '{self._fit_default_dt}' " + f"not found in fdt list: {', '.join(self._fdts)}") + seq = self._fdts.index(default_dt) val = val[1:].replace('DEFAULT-SEQ', str(seq + 1)) fsw.property_string(pname, val) return @@ -711,8 +715,9 @@ class Entry_fit(Entry_section): # Add data for 'images' nodes (but not 'config') if depth == 1 and in_images: if fdt_phase: + leaf = os.path.basename(fdt_fname) phase_fname = tools.get_output_filename( - f'{fdt_fname}-{fdt_phase}.dtb') + f'{leaf}-{fdt_phase}.dtb') self._run_fdtgrep(fname, fdt_phase, phase_fname) data = tools.read_file(phase_fname) else: diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 3f05559501d..b133c76188c 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -4183,7 +4183,8 @@ class TestFunctional(unittest.TestCase): data = self._DoReadFile('172_scp.dts') self.assertEqual(SCP_DATA, data[:len(SCP_DATA)]) - def CheckFitFdt(self, dts='170_fit_fdt.dts', use_fdt_list=True): + def CheckFitFdt(self, dts='170_fit_fdt.dts', use_fdt_list=True, + default_dt=None): """Check an image with an FIT with multiple FDT images""" def _CheckFdt(seq, expected_data): """Check the FDT nodes @@ -4227,6 +4228,8 @@ class TestFunctional(unittest.TestCase): } if use_fdt_list: entry_args['of-list'] = 'test-fdt1 test-fdt2' + if default_dt: + entry_args['default-dt'] = default_dt data = self._DoReadFileDtb( dts, entry_args=entry_args, @@ -7633,6 +7636,16 @@ fdt fdtmap Extract the devicetree blob from the fdtmap finally: os.chdir(old_dir) + def testFitFdtListDirDefault(self): + """Test an FIT fit,fdt-list-dir where the default DT in is a subdir""" + old_dir = os.getcwd() + try: + os.chdir(self._indir) + self.CheckFitFdt('333_fit_fdt_dir.dts', False, + default_dt='rockchip/test-fdt2') + finally: + os.chdir(old_dir) + def testFitFdtCompat(self): """Test an image with an FIT with compatible in the config nodes""" entry_args = { From f2154c30f6174eb27dfe92c91950f6436f802f1b Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Aug 2024 13:11:38 -0600 Subject: [PATCH 12/17] binman: Adjust naming for reading symbols These functions get the value of a symbol. The reference to ELF files is confusing since they are reading the position/size of entries, not ELF symbols. Rename the functions and adjust the comments also. Signed-off-by: Simon Glass --- tools/binman/elf.py | 4 ++-- tools/binman/elf_test.py | 4 ++-- tools/binman/etype/section.py | 13 ++++++------- tools/binman/image.py | 11 +++++------ tools/binman/image_test.py | 8 ++++---- 5 files changed, 19 insertions(+), 21 deletions(-) diff --git a/tools/binman/elf.py b/tools/binman/elf.py index a4694056391..1c3b1e225f4 100644 --- a/tools/binman/elf.py +++ b/tools/binman/elf.py @@ -301,8 +301,8 @@ def LookupAndWriteSymbols(elf_fname, entry, section, is_elf=False, value = BINMAN_SYM_MAGIC_VALUE else: # Look up the symbol in our entry tables. - value = section.GetImage().LookupImageSymbol(name, sym.weak, - msg, base_addr) + value = section.GetImage().GetImageSymbolValue(name, sym.weak, + msg, base_addr) if value is None: value = -1 pack_string = pack_string.lower() diff --git a/tools/binman/elf_test.py b/tools/binman/elf_test.py index b64134123c1..2f22639dffc 100644 --- a/tools/binman/elf_test.py +++ b/tools/binman/elf_test.py @@ -37,7 +37,7 @@ class FakeSection: """A fake Section object, used for testing This has the minimum feature set needed to support testing elf functions. - A LookupSymbol() function is provided which returns a fake value for amu + A GetSymbolValue() function is provided which returns a fake value for any symbol requested. """ def __init__(self, sym_value=1): @@ -46,7 +46,7 @@ class FakeSection: def GetPath(self): return 'section_path' - def LookupImageSymbol(self, name, weak, msg, base_addr): + def GetImageSymbolValue(self, name, weak, msg, base_addr): """Fake implementation which returns the same value for all symbols""" return self.sym_value diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 30c1041c7e8..6c181531b8b 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -563,13 +563,13 @@ class Entry_section(Entry): return entry.GetData(required) def LookupEntry(self, entries, sym_name, msg): - """Look up the entry for an ENF symbol + """Look up the entry for a binman symbol Args: entries (dict): entries to search: key: entry name value: Entry object - sym_name: Symbol name in the ELF file to look up in the format + sym_name: Symbol name to look up in the format _binman__prop_ where is the name of the entry and is the property to find (e.g. _binman_u_boot_prop_offset). As a special case, you can append @@ -606,11 +606,10 @@ class Entry_section(Entry): entry = entries[name] return entry, entry_name, prop_name - def LookupSymbol(self, sym_name, optional, msg, base_addr, entries=None): - """Look up a symbol in an ELF file + def GetSymbolValue(self, sym_name, optional, msg, base_addr, entries=None): + """Get the value of a Binman symbol - Looks up a symbol in an ELF file. Only entry types which come from an - ELF image can be used by this function. + Look up a Binman symbol and obtain its value. At present the only entry properties supported are: offset @@ -618,7 +617,7 @@ class Entry_section(Entry): size Args: - sym_name: Symbol name in the ELF file to look up in the format + sym_name: Symbol name to look up in the format _binman__prop_ where is the name of the entry and is the property to find (e.g. _binman_u_boot_prop_offset). As a special case, you can append diff --git a/tools/binman/image.py b/tools/binman/image.py index 702c9055585..c667f583db6 100644 --- a/tools/binman/image.py +++ b/tools/binman/image.py @@ -381,11 +381,10 @@ class Image(section.Entry_section): selected_entries.append(entry) return selected_entries, lines, widths - def LookupImageSymbol(self, sym_name, optional, msg, base_addr): - """Look up a symbol in an ELF file + def GetImageSymbolValue(self, sym_name, optional, msg, base_addr): + """Get the value of a Binman symbol - Looks up a symbol in an ELF file. Only entry types which come from an - ELF image can be used by this function. + Look up a Binman symbol and obtain its value. This searches through this image including all of its subsections. @@ -423,8 +422,8 @@ class Image(section.Entry_section): entries = OrderedDict() entries_by_name = {} self._CollectEntries(entries, entries_by_name, self) - return self.LookupSymbol(sym_name, optional, msg, base_addr, - entries_by_name) + return self.GetSymbolValue(sym_name, optional, msg, base_addr, + entries_by_name) def CollectBintools(self): """Collect all the bintools used by this image diff --git a/tools/binman/image_test.py b/tools/binman/image_test.py index bd51c1e55d1..7d65e2d589a 100644 --- a/tools/binman/image_test.py +++ b/tools/binman/image_test.py @@ -13,7 +13,7 @@ class TestImage(unittest.TestCase): def testInvalidFormat(self): image = Image('name', 'node', test=True) with self.assertRaises(ValueError) as e: - image.LookupSymbol('_binman_something_prop_', False, 'msg', 0) + image.GetSymbolValue('_binman_something_prop_', False, 'msg', 0) self.assertIn( "msg: Symbol '_binman_something_prop_' has invalid format", str(e.exception)) @@ -22,7 +22,7 @@ class TestImage(unittest.TestCase): image = Image('name', 'node', test=True) image._entries = {} with self.assertRaises(ValueError) as e: - image.LookupSymbol('_binman_type_prop_pname', False, 'msg', 0) + image.GetSymbolValue('_binman_type_prop_pname', False, 'msg', 0) self.assertIn("msg: Entry 'type' not found in list ()", str(e.exception)) @@ -30,7 +30,7 @@ class TestImage(unittest.TestCase): image = Image('name', 'node', test=True) image._entries = {} with capture_sys_output() as (stdout, stderr): - val = image.LookupSymbol('_binman_type_prop_pname', True, 'msg', 0) + val = image.GetSymbolValue('_binman_type_prop_pname', True, 'msg', 0) self.assertEqual(val, None) self.assertEqual("Warning: msg: Entry 'type' not found in list ()\n", stderr.getvalue()) @@ -40,5 +40,5 @@ class TestImage(unittest.TestCase): image = Image('name', 'node', test=True) image._entries = {'u-boot': 1} with self.assertRaises(ValueError) as e: - image.LookupSymbol('_binman_u_boot_prop_bad', False, 'msg', 0) + image.GetSymbolValue('_binman_u_boot_prop_bad', False, 'msg', 0) self.assertIn("msg: No such property 'bad", str(e.exception)) From 01a609930b996499b36418108125ee53ab7094b5 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Aug 2024 13:11:39 -0600 Subject: [PATCH 13/17] binman: Add minor improvements to symbol-writing Add a clarification to the documentation and add a missing comment. Also update the test so that when it fails it is easier to see what is going on, rather than having to decode hex strings. Signed-off-by: Simon Glass --- tools/binman/binman.rst | 4 +++- tools/binman/elf.py | 3 ++- tools/binman/ftest.py | 36 +++++++++++++++++++++++++++++------- 3 files changed, 34 insertions(+), 9 deletions(-) diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst index 0cafc36bdcb..04564f4f66f 100644 --- a/tools/binman/binman.rst +++ b/tools/binman/binman.rst @@ -494,7 +494,9 @@ point into the image. For example, say SPL is at the start of the image and linked to start at address 80108000. If U-Boot's image-pos is 0x8000 then binman will write an image-pos for U-Boot of 80110000 into the SPL binary, since it assumes the image is loaded -to 80108000, with SPL at 80108000 and U-Boot at 80110000. +to 80108000, with SPL at 80108000 and U-Boot at 80110000. In other words, the +positions are calculated relative to the start address of the image to which +they are being written. For x86 devices (with the end-at-4gb property) this base address is not added since it is assumed that images are XIP and the offsets already include the diff --git a/tools/binman/elf.py b/tools/binman/elf.py index 1c3b1e225f4..73394830ebe 100644 --- a/tools/binman/elf.py +++ b/tools/binman/elf.py @@ -247,7 +247,8 @@ def LookupAndWriteSymbols(elf_fname, entry, section, is_elf=False, entry entry: Entry to process section: Section which can be used to lookup symbol values - base_sym: Base symbol marking the start of the image + base_sym: Base symbol marking the start of the image (__image_copy_start + by default) Returns: int: Number of symbols written diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index b133c76188c..7b4454bd342 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -1526,18 +1526,40 @@ class TestFunctional(unittest.TestCase): # The image should contain the symbols from u_boot_binman_syms.c # Note that image_pos is adjusted by the base address of the image, # which is 0x10 in our test image - sym_values = struct.pack(' Date: Mon, 26 Aug 2024 13:11:40 -0600 Subject: [PATCH 14/17] binman: Provide a way to set the symbol base address The base address of the ELF containing symbols is normally added to any symbols written, so that the value points to the correct address in memory when everything is loaded. When the binary resides on disk, a different offset may be needed, typically 0. Provide a way to specify this. Signed-off-by: Simon Glass --- tools/binman/binman.rst | 15 ++++++++ tools/binman/elf.py | 7 ++-- tools/binman/entry.py | 8 ++++- tools/binman/etype/blob_phase.py | 5 +++ tools/binman/ftest.py | 35 ++++++++++++++++--- tools/binman/test/336_symbols_base.dts | 23 ++++++++++++ tools/binman/test/337_symbols_base_expand.dts | 24 +++++++++++++ 7 files changed, 110 insertions(+), 7 deletions(-) create mode 100644 tools/binman/test/336_symbols_base.dts create mode 100644 tools/binman/test/337_symbols_base_expand.dts diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst index 04564f4f66f..f9a3a42183b 100644 --- a/tools/binman/binman.rst +++ b/tools/binman/binman.rst @@ -502,6 +502,10 @@ For x86 devices (with the end-at-4gb property) this base address is not added since it is assumed that images are XIP and the offsets already include the address. +For non-x86 cases where the symbol is used as a flash offset, the symbols-base +property can be set to that offset (e.g. 0), so that the unadjusted image-pos +is written into the image. + While U-Boot's symbol updating is handled automatically by the u-boot-spl entry type (and others), it is possible to use this feature with any blob. To do this, add a `write-symbols` (boolean) property to the node, set the ELF @@ -743,6 +747,17 @@ insert-template: properties are brought into the target node. See Templates_ below for more information. +symbols-base: + When writing symbols into a binary, the value of that symbol is assumed to + be relative to the base address of the binary. This allow the binary to be + loaded in memory at its base address, so that symbols point into the binary + correctly. In some cases the binary is in fact not yet in memory, but must + be read from storage. In this case there is no base address for the symbols. + This property can be set to 0 to indicate this. Other values for + symbols-base are allowed, but care must be taken that the code which uses + the symbol is aware of the base being used. If omitted, the binary's base + address is used. + The attributes supported for images and sections are described below. Several are similar to those for entries. diff --git a/tools/binman/elf.py b/tools/binman/elf.py index 73394830ebe..c75f4478813 100644 --- a/tools/binman/elf.py +++ b/tools/binman/elf.py @@ -234,7 +234,7 @@ def GetSymbolOffset(elf_fname, sym_name, base_sym=None): return val - base def LookupAndWriteSymbols(elf_fname, entry, section, is_elf=False, - base_sym=None): + base_sym=None, base_addr=None): """Replace all symbols in an entry with their correct values The entry contents is updated so that values for referenced symbols will be @@ -249,6 +249,8 @@ def LookupAndWriteSymbols(elf_fname, entry, section, is_elf=False, section: Section which can be used to lookup symbol values base_sym: Base symbol marking the start of the image (__image_copy_start by default) + base_addr (int): Base address to use for the entry being written. If + None then the value of base_sym is used Returns: int: Number of symbols written @@ -278,7 +280,8 @@ def LookupAndWriteSymbols(elf_fname, entry, section, is_elf=False, if not base and not is_elf: tout.debug(f'LookupAndWriteSymbols: no base: elf_fname={elf_fname}, base_sym={base_sym}, is_elf={is_elf}') return 0 - base_addr = 0 if is_elf else base.address + if base_addr is None: + base_addr = 0 if is_elf else base.address count = 0 for name, sym in syms.items(): if name.startswith('_binman'): diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 7d4d4692776..8ad9fe89e0c 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -108,6 +108,9 @@ class Entry(object): not need to be done again. This is only used with 'binman replace', to stop sections from being rebuilt if their entries have not been replaced + symbols_base (int): Use this value as the assumed load address of the + target entry, when calculating the symbol value. If None, this is + 0 for blobs and the image-start address for ELF files """ fake_dir = None @@ -159,6 +162,7 @@ class Entry(object): self.preserve = False self.build_done = False self.no_write_symbols = False + self.symbols_base = None @staticmethod def FindEntryClass(etype, expanded): @@ -324,6 +328,7 @@ class Entry(object): self.preserve = fdt_util.GetBool(self._node, 'preserve') self.no_write_symbols = fdt_util.GetBool(self._node, 'no-write-symbols') + self.symbols_base = fdt_util.GetInt(self._node, 'symbols-base') def GetDefaultFilename(self): return None @@ -713,7 +718,8 @@ class Entry(object): # Check if we are writing symbols into an ELF file is_elf = self.GetDefaultFilename() == self.elf_fname elf.LookupAndWriteSymbols(self.elf_fname, self, section.GetImage(), - is_elf, self.elf_base_sym) + is_elf, self.elf_base_sym, + self.symbols_base) def CheckEntries(self): """Check that the entry offsets are correct diff --git a/tools/binman/etype/blob_phase.py b/tools/binman/etype/blob_phase.py index 951d9934050..09bb89b3b78 100644 --- a/tools/binman/etype/blob_phase.py +++ b/tools/binman/etype/blob_phase.py @@ -57,3 +57,8 @@ class Entry_blob_phase(Entry_section): if self.no_write_symbols: for entry in self._entries.values(): entry.no_write_symbols = True + + # Propagate the symbols-base property + if self.symbols_base is not None: + for entry in self._entries.values(): + entry.symbols_base = self.symbols_base diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 7b4454bd342..ecfcd6bd915 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -1500,7 +1500,8 @@ class TestFunctional(unittest.TestCase): self.assertEqual(U_BOOT_SPL_NODTB_DATA, data[:len(U_BOOT_SPL_NODTB_DATA)]) def checkSymbols(self, dts, base_data, u_boot_offset, entry_args=None, - use_expanded=False, no_write_symbols=False): + use_expanded=False, no_write_symbols=False, + symbols_base=None): """Check the image contains the expected symbol values Args: @@ -1512,6 +1513,8 @@ class TestFunctional(unittest.TestCase): value: value of that arg use_expanded: True to use expanded entries where available, e.g. 'u-boot-expanded' instead of 'u-boot' + symbols_base (int): Value to expect for symbols-base in u-boot-spl, + None if none """ elf_fname = self.ElfTestFile('u_boot_binman_syms') syms = elf.GetSymbols(elf_fname, ['binman', 'image']) @@ -1526,10 +1529,19 @@ class TestFunctional(unittest.TestCase): # The image should contain the symbols from u_boot_binman_syms.c # Note that image_pos is adjusted by the base address of the image, # which is 0x10 in our test image - vals = (elf.BINMAN_SYM_MAGIC_VALUE, 0x00, + vals2 = (elf.BINMAN_SYM_MAGIC_VALUE, 0x00, u_boot_offset + len(U_BOOT_DATA), 0x10 + u_boot_offset, 0x04) + + # u-boot-spl has a symbols-base property, so take that into account if + # required. The caller must supply the value + vals = list(vals2) + if symbols_base is not None: + vals[3] = symbols_base + u_boot_offset + vals = tuple(vals) + sym_values = struct.pack('; + #size-cells = <1>; + + binman { + pad-byte = <0xff>; + u-boot-spl { + symbols-base = <0>; + }; + + u-boot { + offset = <0x1c>; + }; + + u-boot-spl2 { + type = "u-boot-spl"; + }; + }; +}; diff --git a/tools/binman/test/337_symbols_base_expand.dts b/tools/binman/test/337_symbols_base_expand.dts new file mode 100644 index 00000000000..5a777ae63b8 --- /dev/null +++ b/tools/binman/test/337_symbols_base_expand.dts @@ -0,0 +1,24 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + pad-byte = <0xff>; + u-boot-spl { + symbols-base = <0>; + }; + + u-boot { + offset = <0x38>; + no-expanded; + }; + + u-boot-spl2 { + type = "u-boot-spl"; + }; + }; +}; From b73d0bb584e5f89c8d80c7435f1a6c036be25dd9 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Aug 2024 13:11:41 -0600 Subject: [PATCH 15/17] binman: Unwind the end-at-4gb special-case a little Move the check for this further out, so that base_addr is computed in Entry.WriteSymbols() rather than at lower levels. Signed-off-by: Simon Glass --- tools/binman/entry.py | 11 +++++++++-- tools/binman/etype/section.py | 15 +++++---------- tools/binman/image.py | 10 ++++------ 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 8ad9fe89e0c..68f8d62bba9 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -711,15 +711,22 @@ class Entry(object): def WriteSymbols(self, section): """Write symbol values into binary files for access at run time + As a special case, if symbols_base is not specified and this is an + end-at-4gb image, a symbols_base of 0 is used + Args: section: Section containing the entry """ if self.auto_write_symbols and not self.no_write_symbols: # Check if we are writing symbols into an ELF file is_elf = self.GetDefaultFilename() == self.elf_fname + + symbols_base = self.symbols_base + if symbols_base is None and self.GetImage()._end_4gb: + symbols_base = 0 + elf.LookupAndWriteSymbols(self.elf_fname, self, section.GetImage(), - is_elf, self.elf_base_sym, - self.symbols_base) + is_elf, self.elf_base_sym, symbols_base) def CheckEntries(self): """Check that the entry offsets are correct diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 6c181531b8b..7d2eb42627d 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -627,12 +627,10 @@ class Entry_section(Entry): 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 - base_addr: Base address of image. This is added to the returned - image_pos in most cases so that the returned position indicates - where the targetted entry/binary has actually been loaded. But - if end-at-4gb is used, this is not done, since the binary is - already assumed to be linked to the ROM position and using - execute-in-place (XIP). + base_addr (int): Base address of image. This is added to the + returned value of image-pos so that the returned position + indicates where the targeted entry/binary has actually been + loaded Returns: Value that should be assigned to that symbol, or None if it was @@ -655,10 +653,7 @@ class Entry_section(Entry): if prop_name == 'offset': return entry.offset elif prop_name == 'image_pos': - value = entry.image_pos - if not self.GetImage()._end_4gb: - value += base_addr - return value + return base_addr + entry.image_pos if prop_name == 'size': return entry.size else: diff --git a/tools/binman/image.py b/tools/binman/image.py index c667f583db6..24ce0af7c72 100644 --- a/tools/binman/image.py +++ b/tools/binman/image.py @@ -404,12 +404,10 @@ class Image(section.Entry_section): 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 - base_addr: Base address of image. This is added to the returned - image_pos in most cases so that the returned position indicates - where the targeted entry/binary has actually been loaded. But - if end-at-4gb is used, this is not done, since the binary is - already assumed to be linked to the ROM position and using - execute-in-place (XIP). + base_addr (int): Base address of image. This is added to the + returned value of image-pos so that the returned position + indicates where the targeted entry/binary has actually been + loaded Returns: Value that should be assigned to that symbol, or None if it was From a96dda1a70afc348d472cfb087742604e50b8d46 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Aug 2024 13:11:42 -0600 Subject: [PATCH 16/17] binman: Allow image_pos to be None when writing symbols Some images do not have an image_pos value, for example an image which is part of a compressed section and therefore cannot be accessed directly. Handle this case, returning None as the value. Signed-off-by: Simon Glass --- tools/binman/etype/section.py | 3 ++ tools/binman/ftest.py | 49 +++++++++++++++++++------- tools/binman/test/338_symbols_comp.dts | 26 ++++++++++++++ 3 files changed, 65 insertions(+), 13 deletions(-) create mode 100644 tools/binman/test/338_symbols_comp.dts diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 7d2eb42627d..f4f48c00e87 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -653,6 +653,9 @@ class Entry_section(Entry): if prop_name == 'offset': return entry.offset elif prop_name == 'image_pos': + if not entry.image_pos: + tout.info(f'Symbol-writing: no value for {entry._node.path}') + return None return base_addr + entry.image_pos if prop_name == 'size': return entry.size diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index ecfcd6bd915..58f9d8256e8 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -505,8 +505,9 @@ class TestFunctional(unittest.TestCase): return dtb.GetContents() def _DoReadFileDtb(self, fname, use_real_dtb=False, use_expanded=False, - map=False, update_dtb=False, entry_args=None, - reset_dtbs=True, extra_indirs=None, threads=None): + verbosity=None, map=False, update_dtb=False, + entry_args=None, reset_dtbs=True, extra_indirs=None, + threads=None): """Run binman and return the resulting image This runs binman with a given test file and then reads the resulting @@ -523,6 +524,7 @@ class TestFunctional(unittest.TestCase): But in some test we need the real contents. use_expanded: True to use expanded entries where available, e.g. 'u-boot-expanded' instead of 'u-boot' + verbosity: Verbosity level to use (0-3, None=don't set it) map: True to output map files for the images update_dtb: Update the offset and size of each entry in the device tree before packing it into the image @@ -559,7 +561,8 @@ class TestFunctional(unittest.TestCase): try: retcode = self._DoTestFile(fname, map=map, update_dtb=update_dtb, entry_args=entry_args, use_real_dtb=use_real_dtb, - use_expanded=use_expanded, extra_indirs=extra_indirs, + use_expanded=use_expanded, verbosity=verbosity, + extra_indirs=extra_indirs, threads=threads) self.assertEqual(0, retcode) out_dtb_fname = tools.get_output_filename('u-boot.dtb.out') @@ -1507,7 +1510,8 @@ class TestFunctional(unittest.TestCase): Args: dts: Device tree file to use for test base_data: Data before and after 'u-boot' section - u_boot_offset: Offset of 'u-boot' section in image + u_boot_offset (int): Offset of 'u-boot' section in image, or None if + the offset not available due to it being in a compressed section entry_args: Dict of entry args to supply to binman key: arg name value: value of that arg @@ -1525,13 +1529,20 @@ class TestFunctional(unittest.TestCase): self._SetupSplElf('u_boot_binman_syms') data = self._DoReadFileDtb(dts, entry_args=entry_args, - use_expanded=use_expanded)[0] + use_expanded=use_expanded, + verbosity=None if u_boot_offset else 3)[0] + + # The lz4-compressed version of the U-Boot data is 19 bytes long + comp_uboot_len = 19 + # The image should contain the symbols from u_boot_binman_syms.c # Note that image_pos is adjusted by the base address of the image, # which is 0x10 in our test image + # If u_boot_offset is None, Binman should write -1U into the image vals2 = (elf.BINMAN_SYM_MAGIC_VALUE, 0x00, - u_boot_offset + len(U_BOOT_DATA), - 0x10 + u_boot_offset, 0x04) + u_boot_offset + len(U_BOOT_DATA) if u_boot_offset else + len(U_BOOT_SPL_DATA) + 1 + comp_uboot_len, + 0x10 + u_boot_offset if u_boot_offset else 0xffffffff, 0x04) # u-boot-spl has a symbols-base property, so take that into account if # required. The caller must supply the value @@ -1561,17 +1572,21 @@ class TestFunctional(unittest.TestCase): self.assertEqual(base_data[24:], data[24:blen]) self.assertEqual(0xff, data[blen]) - ofs = blen + 1 + len(U_BOOT_DATA) - self.assertEqual(U_BOOT_DATA, data[blen + 1:ofs]) + if u_boot_offset: + ofs = blen + 1 + len(U_BOOT_DATA) + self.assertEqual(U_BOOT_DATA, data[blen + 1:ofs]) + else: + ofs = blen + 1 + comp_uboot_len self.assertEqual(sym_values2, data[ofs:ofs + 24]) self.assertEqual(base_data[24:], data[ofs + 24:]) # Just repeating the above asserts all at once, for clarity - expected = (sym_values + base_data[24:] + - tools.get_bytes(0xff, 1) + U_BOOT_DATA + sym_values2 + - base_data[24:]) - self.assertEqual(expected, data) + if u_boot_offset: + expected = (sym_values + base_data[24:] + + tools.get_bytes(0xff, 1) + U_BOOT_DATA + + sym_values2 + base_data[24:]) + self.assertEqual(expected, data) def testSymbols(self): """Test binman can assign symbols embedded in U-Boot""" @@ -7777,6 +7792,14 @@ fdt fdtmap Extract the devicetree blob from the fdtmap entry_args=entry_args, use_expanded=True, symbols_base=0) + def testSymbolsCompressed(self): + """Test binman complains about symbols from a compressed section""" + with test_util.capture_sys_output() as (stdout, stderr): + self.checkSymbols('338_symbols_comp.dts', U_BOOT_SPL_DATA, None) + out = stdout.getvalue() + self.assertIn('Symbol-writing: no value for /binman/section/u-boot', + out) + if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/338_symbols_comp.dts b/tools/binman/test/338_symbols_comp.dts new file mode 100644 index 00000000000..15008507cfd --- /dev/null +++ b/tools/binman/test/338_symbols_comp.dts @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + pad-byte = <0xff>; + u-boot-spl { + }; + + section { + offset = <0x1c>; + compress = "lz4"; + + u-boot { + }; + }; + + u-boot-spl2 { + type = "u-boot-spl"; + }; + }; +}; From ac0876c890c2dcaa4e21ce36bfa2ea3e02139a01 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Aug 2024 13:11:43 -0600 Subject: [PATCH 17/17] binman: Make a start on an iMX8 test This patch is for Marek, to provide a starting point. To try it, use 'binman test -T' and see the missing coverage. Signed-off-by: Simon Glass --- tools/binman/etype/nxp_imx8mimage.py | 3 ++- tools/binman/ftest.py | 4 ++++ tools/binman/test/339_nxp_imx8.dts | 17 +++++++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 tools/binman/test/339_nxp_imx8.dts diff --git a/tools/binman/etype/nxp_imx8mimage.py b/tools/binman/etype/nxp_imx8mimage.py index 3585120b79b..8ad177b3b65 100644 --- a/tools/binman/etype/nxp_imx8mimage.py +++ b/tools/binman/etype/nxp_imx8mimage.py @@ -27,7 +27,8 @@ class Entry_nxp_imx8mimage(Entry_mkimage): def __init__(self, section, etype, node): super().__init__(section, etype, node) - self.required_props = ['nxp,boot-from', 'nxp,rom-version', 'nxp,loader-address'] + self.required_props = ['nxp,boot-from', 'nxp,rom-version', + 'nxp,loader-address'] def ReadNode(self): super().ReadNode() diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 58f9d8256e8..e3f231e4bcc 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -7800,6 +7800,10 @@ fdt fdtmap Extract the devicetree blob from the fdtmap self.assertIn('Symbol-writing: no value for /binman/section/u-boot', out) + def testNxpImx8Image(self): + """Test that binman can produce an iMX8 image""" + self._DoTestFile('339_nxp_imx8.dts') + if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/339_nxp_imx8.dts b/tools/binman/test/339_nxp_imx8.dts new file mode 100644 index 00000000000..cb512ae9aa2 --- /dev/null +++ b/tools/binman/test/339_nxp_imx8.dts @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + nxp-imx8mimage { + args; /* TODO: Needed by mkimage etype superclass */ + nxp,boot-from = "sd"; + nxp,rom-version = <1>; + nxp,loader-address = <0x10>; + }; + }; +};