From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id D9ED0C433F5 for ; Thu, 3 Mar 2022 21:17:33 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id C617A83D14; Thu, 3 Mar 2022 22:16:22 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="R5bvP3RY"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 985D183A3B; Thu, 3 Mar 2022 22:16:17 +0100 (CET) Received: from mail-ej1-x632.google.com (mail-ej1-x632.google.com [IPv6:2a00:1450:4864:20::632]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 3A2BD83CA4 for ; Thu, 3 Mar 2022 22:16:03 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=alpernebiyasak@gmail.com Received: by mail-ej1-x632.google.com with SMTP id kt27so13476835ejb.0 for ; Thu, 03 Mar 2022 13:16:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=message-id:date:mime-version:user-agent:from:subject:to:cc :references:content-language:in-reply-to:content-transfer-encoding; bh=kdtY6DpQMSfFdFt/UaYvE2hX9x/5WCVJkbyUcHMgKMM=; b=R5bvP3RYdExfPByJ4uWr2o838pMfbCiLvz4OK1wUJdgvgWtDj7lVj48dGCopMYBnSU Gyi685EljgA9ds5nM7JeSvhdUQeujiKQUQdXib9EXe6imH0hrk19mAd9afDSvCsPEiWX hTKUxyl+L5vPoQNv+gaj9IPuwowe8pzOcaWXDEgRcO3fttRUdPQe2WaLQRRzcb0TnFRV xnhpzg4PXJuRhIi9offJ0ckRJOvetQlZ+Tb92nGtlrX9ePaHQ8w9GlCnnZHcl3Gd/jiI DIKnvJkexc9X8ILZOYSaa1gAvwgUMmmMFKdXu6eEG6p07rOyZChTgW5kcsRVIVgI0LZT YIfA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:from :subject:to:cc:references:content-language:in-reply-to :content-transfer-encoding; bh=kdtY6DpQMSfFdFt/UaYvE2hX9x/5WCVJkbyUcHMgKMM=; b=EU/igXtaZduz9M7DB7GhDm5tg6G3+sKBQWzcpHjlPbI/3kIcDE0K6JzvzKdXY7kywE r+JHjnjenSQ5SExN3uybplZRcapYZ531jS9hzSmlrgEPuib6Y2jZnLlREhqdVFBjnL+w sHg9Nb7uw6ev4UPMNMpT5ZYH2Dy0+IpSDUHylNm1Pk6HD5hrI0KOVo1WhC1t8l7bNNJE dgV/aXiFKy0ha/DsYFzp0SlvhlkS2iLSERyTVAkE6YaCflnfOD8Rs6KsmrII35NOnfjP HnPh8aWeqj1ZIAWVWRvHGq72wjpeQiwA+bA5vRVDFmF7G5UJbJCk4IE0FhyILK639gu5 bKKQ== X-Gm-Message-State: AOAM532QhLURFHD+mvPAUAth6GBhKsdTybt9IEFS799Q5Yq5x+PXuxpe Na/qrgK+qlAqPzcnAnXrN10= X-Google-Smtp-Source: ABdhPJwmnvE+3nIAJlnbbJChAol76FbYhNyYbVU9AxETO1L1F52UPWytcYZ42T5pjPeoh07l0T7CAQ== X-Received: by 2002:a17:907:96a7:b0:6da:7d95:58b0 with SMTP id hd39-20020a17090796a700b006da7d9558b0mr6256900ejc.475.1646342162767; Thu, 03 Mar 2022 13:16:02 -0800 (PST) Received: from [192.168.0.74] ([178.233.26.119]) by smtp.gmail.com with ESMTPSA id l2-20020a1709060cc200b006d3d91e88c7sm1099109ejh.214.2022.03.03.13.16.01 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 03 Mar 2022 13:16:02 -0800 (PST) Message-ID: <2e029792-0786-502c-273e-b92b67e95ea2@gmail.com> Date: Fri, 4 Mar 2022 00:09:12 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:91.0) Gecko/20100101 Thunderbird/91.6.1 From: Alper Nebi Yasak Subject: Re: [PATCH v2 10/25] binman: Refactor fit to generate output at the end To: Simon Glass Cc: Ivan Mikhaylov , Tom Rini , Philippe Reynes , huang lin , Jeffy Chen , Kever Yang , U-Boot Mailing List References: <20220223230040.159317-1-sjg@chromium.org> <20220223230040.159317-11-sjg@chromium.org> Content-Language: en-US In-Reply-To: <20220223230040.159317-11-sjg@chromium.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean On 24/02/2022 02:00, Simon Glass wrote: > At present the fit implementation creates the output tree while > scanning the FIT description. Then it updates the tree later when the > data is known. > > This works, but is a bit confusing, since it requires mixing the scanning > code with the generation code, with a fix-up step at the end. > > It is actually possible to do this in two phases, one to scan everything > and the other to generate the FIT. Thus the FIT is generated in one pass, > when everything is known. Doing it in one go makes sense to me as well. In general I like the way distinct processing actions/steps are being split into their own blocks or so, and I think this helps move things toward that. > Update the code accordingly. The only functional change is that the 'data' > property for each node are now last instead of first, which is really a > more natural position. Update the affected test to deal with this. > > One wrinkle is that the calculated properties (image-pos, size and offset) > are now added before the FIT is generated. so we must filter these out > when copying properties from the binman description to the FIT. > > Most of the change here is splitting out some of the code from the > ReadEntries() implementation into _BuildInput(). So despite the large > diff, most of the code is the same. It is not feasible to split this patch > up, so far as I can tell. > > Signed-off-by: Simon Glass > --- > > Changes in v2: > - Add new patch to refactor fit to generate output at the end > > tools/binman/etype/fit.py | 178 ++++++++++++++----------- > tools/binman/ftest.py | 13 +- > tools/binman/test/224_fit_bad_oper.dts | 2 - > 3 files changed, 109 insertions(+), 84 deletions(-) Reviewed-by: Alper Nebi Yasak I still wrote some weird ideas below, mostly for the future, since this patch is mostly moving code around which is fine as is. > diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py > index 2d4c5f6545..61c72780e9 100644 > --- a/tools/binman/etype/fit.py > +++ b/tools/binman/etype/fit.py > @@ -209,6 +209,81 @@ class Entry_fit(Entry_section): > return oper > > def ReadEntries(self): > + def _add_entries(base_node, depth, node): > + """Add entries for any nodes that need them > + > + Args: > + base_node: Base Node of the FIT (with 'description' property) > + depth: Current node depth (0 is the base 'fit' node) > + node: Current node to process > + > + He we only need to provide binman entries which are used to define He -> Here ? > + the 'data' for each image. We create an entry_Section for each. > + """ > + rel_path = node.path[len(base_node.path):] > + in_images = rel_path.startswith('/images') > + has_images = depth == 2 and in_images > + if has_images: > + # This node is a FIT subimage node (e.g. "/images/kernel") > + # containing content nodes. We collect the subimage nodes and > + # section entries for them here to merge the content subnodes > + # together and put the merged contents in the subimage node's > + # 'data' property later. > + entry = Entry.Create(self.section, node, etype='section') > + entry.ReadNode() I plan to change 'self.section' to 'self' here later, fixes extracting wrong contents for FIT subentries. > + # The hash subnodes here are for mkimage, not binman. > + entry.SetUpdateHash(False) > + self._entries[rel_path] = entry > + I also plan to change this to a single-level node name instead of the relative path, lets 'binman extract fit/u-boot' etc. run at all. > + for subnode in node.subnodes: > + _add_entries(base_node, depth + 1, subnode) > + > + _add_entries(self._node, 0, self._node) I think it's especially visible here what I meant by switching away from recursion: this recurses through every node but only does anything on immediate subnodes of "/images" (for now?). > + > + def BuildSectionData(self, required): > + """Build FIT entry contents > + > + This adds the 'data' properties to the input ITB (Image-tree Binary) > + then runs mkimage to process it. > + > + Args: > + required: True if the data must be present, False if it is OK to > + return None I forgot to handle 'required' while converting FIT to section... > + > + Returns: > + Contents of the section (bytes) > + """ > + data = self._BuildInput() > + uniq = self.GetUniqueName() > + input_fname = tools.get_output_filename('%s.itb' % uniq) > + output_fname = tools.get_output_filename('%s.fit' % uniq) > + tools.write_file(input_fname, data) > + tools.write_file(output_fname, data) > + > + args = {} > + ext_offset = self._fit_props.get('fit,external-offset') > + if ext_offset is not None: > + args = { > + 'external': True, > + 'pad': fdt_util.fdt32_to_cpu(ext_offset.value) > + } > + if self.mkimage.run(reset_timestamp=True, output_fname=output_fname, > + **args) is None: I have an idea for the far future, to let /image/* nodes sometimes be Entry_collection to handle external offsets in binman so we can take mkimage completely out of this, but no clue how feasible/desirable that end goal is. > + # Bintool is missing; just use empty data as the output > + self.record_missing_bintool(self.mkimage) > + return tools.get_bytes(0, 1024) > + > + return tools.read_file(output_fname) > + > + def _BuildInput(self): > + """Finish the FIT by adding the 'data' properties to it > + > + Arguments: > + fdt: FIT to update > + > + Returns: > + New fdt contents (bytes) > + """ > def _process_prop(pname, prop): > """Process special properties > > @@ -236,9 +311,15 @@ class Entry_fit(Entry_section): > val = val[1:].replace('DEFAULT-SEQ', str(seq + 1)) > fsw.property_string(pname, val) > return > + elif pname.startswith('fit,'): > + # Ignore these, which are commands for binman to process > + return > + elif pname in ['offset', 'size', 'image-pos']: > + # Don't add binman's calculated properties > + return This is one of the things I was thinking of doing, thanks. I encountered the same issue when replacing a FIT entry with the same f"{uniq}.fit" that was used to build it, will try adding a test for that later. > fsw.property(pname, prop.bytes) > > - def _scan_gen_fdt_nodes(subnode, depth, in_images): > + def _gen_fdt_nodes(subnode, depth, in_images): > """Generate FDT nodes > > This creates one node for each member of self._fdts using the > [...] > > diff --git a/tools/binman/test/224_fit_bad_oper.dts b/tools/binman/test/224_fit_bad_oper.dts > index cee801e2ea..8a8014ea33 100644 > --- a/tools/binman/test/224_fit_bad_oper.dts > +++ b/tools/binman/test/224_fit_bad_oper.dts > @@ -21,7 +21,5 @@ > }; > }; > }; > - fdtmap { > - }; This looked unrelated at first, but as I understand it, changing the order of things made the bad-operation error happen later and exposed a breakage from the fdtmap entry trying to parse the mock dtb data. > }; > };