Re: [PATCH 12/12] binman: Support simple templates

7 Jul 2023


      On 07.07.23 19:35, Simon Glass wrote:
...
Hi Jan,
On Fri, 7 Jul 2023 at 17:03, Jan Kiszka jan.kiszka@siemens.com wrote:
...
On 07.07.23 17:35, Simon Glass wrote:
...
Hi Jan,
On Fri, 7 Jul 2023 at 14:56, Jan Kiszka jan.kiszka@siemens.com wrote:
...
On 07.07.23 14:42, Simon Glass wrote:
...
Hi Jan,
On Fri, 7 Jul 2023 at 11:05, Jan Kiszka jan.kiszka@siemens.com wrote:
...
On 05.07.23 00:14, Simon Glass wrote:
> Hi Jan,
>
> On Mon, 3 Jul 2023 at 20:34, Jan Kiszka jan.kiszka@siemens.com wrote:
>>
>> Hi Simon,
>>
>> On 28.06.23 13:41, Simon Glass wrote:
>>> Collections can used to collect the contents of other entries into a
>>> single entry, but they result in a single entry, with the original entries
>>> 'left behind' in their old place.
>>>
>>> It is useful to be able to specific a set of entries ones and have it used
>>> in multiple images, or parts of an image.
>>>
>>> Implement this mechanism.
>>>
>>> Signed-off-by: Simon Glass sjg@chromium.org
>>> ---
>>>
>>>  tools/binman/binman.rst                  | 80 ++++++++++++++++++++++++
>>>  tools/binman/control.py                  |  9 +++
>>>  tools/binman/etype/section.py            |  3 +-
>>>  tools/binman/ftest.py                    |  8 +++
>>>  tools/binman/test/286_entry_template.dts | 42 +++++++++++++
>>>  5 files changed, 141 insertions(+), 1 deletion(-)
>>>  create mode 100644 tools/binman/test/286_entry_template.dts
>>>
>>> diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst
>>> index a4b31fe5397b..9be979ae1c5b 100644
>>> --- a/tools/binman/binman.rst
>>> +++ b/tools/binman/binman.rst
>>> @@ -727,6 +727,13 @@ optional:
>>>      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.
>>>
>>> +insert-template:
>>> +    This is not strictly speaking an entry property, since it is processed early
>>> +    in Binman before the entries are read. It is a list of phandles of nodes to
>>> +    include in the current (target) node. For each node, its subnodes and their
>>> +    properties are brought into the target node. See Templates_ below for
>>> +    more information.
>>> +
>>>  The attributes supported for images and sections are described below. Several
>>>  are similar to those for entries.
>>>
>>> @@ -1172,6 +1179,79 @@ If you are having trouble figuring out what is going on, you can use
>>>       arch/arm/dts/u-boot.dtsi ... found: "arch/arm/dts/juno-r2-u-boot.dtsi"
>>>
>>>
>>> +Templates
>>> +=========
>>> +
>>> +Sometimes multiple images need to be created which have all have a common
>>> +part. For example, a board may generate SPI and eMMC images which both include
>>> +a FIT. Since the FIT includes many entries, it is tedious to repeat them twice
>>> +in the image description.
>>> +
>>> +Templates provide a simple way to handle this::
>>> +
>>> +    binman {
>>> +        multiple-images;
>>> +        common_part: template-1 {
>>> +            fit {
>>> +                ... lots of entries in here
>>> +            };
>>> +
>>> +            text {
>>> +                text = "base image";
>>> +            };
>>> +        };
>>> +
>>> +        spi-image {
>>> +            filename = "image-spi.bin";
>>> +            insert-template = <&fit>;
>>> +
>>> +            /* things specific to SPI follow */
>>> +            header {
>>> +            ];
>>> +
>>> +            text {
>>> +                text = "SPI image";
>>> +            };
>>> +        };
>>> +
>>> +        mmc-image {
>>> +            filename = "image-mmc.bin";
>>> +            insert-template = <&fit>;
>>> +
>>> +            /* things specific to MMC follow */
>>> +            header {
>>> +            ];
>>> +
>>> +            text {
>>> +                text = "MMC image";
>>> +            };
>>> +        };
>>> +    };
>>> +
>>> +The template node name must start with 'template', so it is not considered to be
>>> +an image itself.
>>> +
>>> +The mechanism is very simple. For each phandle in the 'insert-templates'
>>> +property, the source node is looked up. Then the subnodes of that source node
>>> +are copied into the target node, i.e. the one containing the `insert-template`
>>> +property.
>>> +
>>> +If the target node has a node with the same name as a template, its properties
>>> +override corresponding properties in the template. This allows the template to
>>> +be uses as a base, with the node providing updates to the properties as needed.
>>> +The overriding happens recursively.
>>> +
>>> +At present there is an unpleasant limitation on this feature: it works by
>>> +appending the template nodes after any existing subnodes to the target node.
>>> +This means that if the target node includes any subnodes, these appear in order
>>> +before the template node. In the above example, 'header' becomes the first
>>> +subnode of each image, followed by `fit` and `text`. If this is not what is
>>> +desired, there is no way to adjust it.
>>> +
>>> +Note: The above limitation will likely be removed in future, so that the
>>> +template subnodes appear before the target subnodes.
>>> +
>>> +
>>>  Updating an ELF file
>>>  ====================
>>>
>>> diff --git a/tools/binman/control.py b/tools/binman/control.py
>>> index 68597c4e7792..e7faca78e9aa 100644
>>> --- a/tools/binman/control.py
>>> +++ b/tools/binman/control.py
>>> @@ -22,6 +22,7 @@ from binman import bintool
>>>  from binman import cbfs_util
>>>  from binman import elf
>>>  from binman import entry
>>> +from dtoc import fdt_util
>>>  from u_boot_pylib import command
>>>  from u_boot_pylib import tools
>>>  from u_boot_pylib import tout
>>> @@ -478,6 +479,12 @@ def SignEntries(image_fname, input_fname, privatekey_fname, algo, entry_paths,
>>>
>>>      AfterReplace(image, allow_resize=True, write_map=write_map)
>>>
>>> +def _ProcessTemplates(parent):
>>> +    for node in parent.subnodes:
>>> +        tmpl = fdt_util.GetPhandleList(node, 'insert-template')
>>> +        if tmpl:
>>> +            node.copy_subnodes_from_phandles(tmpl)
>>> +
>>>  def PrepareImagesAndDtbs(dtb_fname, select_images, update_fdt, use_expanded):
>>>      """Prepare the images to be processed and select the device tree
>>>
>>> @@ -520,6 +527,8 @@ def PrepareImagesAndDtbs(dtb_fname, select_images, update_fdt, use_expanded):
>>>          raise ValueError("Device tree '%s' does not have a 'binman' "
>>>                              "node" % dtb_fname)
>>>
>>> +    _ProcessTemplates(node)
>>> +
>>>      images = _ReadImageDesc(node, use_expanded)
>>>
>>>      if select_images:
>>> diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
>>> index d56cc11d1023..adac2ff7fa87 100644
>>> --- a/tools/binman/etype/section.py
>>> +++ b/tools/binman/etype/section.py
>>> @@ -179,7 +179,8 @@ class Entry_section(Entry):
>>>          Returns:
>>>              bool: True if the node is a special one, else False
>>>          """
>>> -        return node.name.startswith('hash') or node.name.startswith('signature')
>>> +        start_list = ('hash', 'signature', 'template')
>>> +        return any(node.name.startswith(name) for name in start_list)
>>>
>>>      def ReadNode(self):
>>>          """Read properties from the section node"""
>>> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
>>> index 4db54c69682c..9d9e47ce26b0 100644
>>> --- a/tools/binman/ftest.py
>>> +++ b/tools/binman/ftest.py
>>> @@ -6763,6 +6763,14 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
>>>          data = self._DoReadFileDtb('285_spl_expand.dts',
>>>                                     use_expanded=True, entry_args=entry_args)[0]
>>>
>>> +    def testEntryTemplate(self):
>>> +        """Test using a template"""
>>> +        TestFunctional._MakeInputFile('vga2.bin', b'#' + VGA_DATA)
>>> +        data = self._DoReadFile('286_entry_template.dts')
>>> +        first = U_BOOT_DTB_DATA + U_BOOT_DATA + VGA_DATA
>>> +        second = U_BOOT_DTB_DATA + b'#' + VGA_DATA + U_BOOT_DATA
>>> +        self.assertEqual(U_BOOT_IMG_DATA + first + second, data)
>>> +
>>>
>>>  if __name__ == "__main__":
>>>      unittest.main()
>>> diff --git a/tools/binman/test/286_entry_template.dts b/tools/binman/test/286_entry_template.dts
>>> new file mode 100644
>>> index 000000000000..6980dbfafcc6
>>> --- /dev/null
>>> +++ b/tools/binman/test/286_entry_template.dts
>>> @@ -0,0 +1,42 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +
>>> +/dts-v1/;
>>> +
>>> +/ {
>>> +     #address-cells = <1>;
>>> +     #size-cells = <1>;
>>> +
>>> +     binman {
>>> +             u-boot-img {
>>> +             };
>>> +
>>> +             common_part: template {
>>> +                     u-boot {
>>> +                     };
>>> +
>>> +                     intel-vga {
>>> +                             filename = "vga.bin";
>>> +                     };
>>> +             };
>>> +
>>> +             first {
>>> +                     type = "section";
>>> +                     insert-template = <&common_part>;
>>> +
>>> +                     u-boot-dtb {
>>> +                     };
>>> +             };
>>> +
>>> +             second {
>>> +                     type = "section";
>>> +                     insert-template = <&common_part>;
>>> +
>>> +                     u-boot-dtb {
>>> +                     };
>>> +
>>> +                     intel-vga {
>>> +                             filename = "vga2.bin";
>>> +                     };
>>> +             };
>>> +     };
>>> +};
>>
>> I tried to apply this on the IOT2050 use case, but it failed in two cases:
>>
>>
>> 1. We need a template where nodes can be manipulated afterwards, thus we
>> need anchors into those nodes. Naturally, those anchors need to get
>> unique names so that something like this is possible:
>>
>> common: template {
>>         anchor: some-node {
>>                 value-1 = <1>;
>>         };
>> };
>>
>> user-a {
>>         insert-template = <&common>;
>> };
>>
>> user-b {
>>         insert-template = <&common>;
>> };
>>
>> &anchor-a {
>>         value-2 = <2>;
>> };
>>
>> &anchor-b {
>>         value-2 = <3>;
>> };
>
> Rather than using a phandle, can you not use the node name in the
> template? For example:
>
> user-a {
>    insert-template = <&common>;
>    some-node {
>       value2 = <2>;
>   }
>
> user-b {
>    insert-template = <&common>;
>    some-node {
>       value2 = <3>;
>   }
>
I don't think this is working already. This is what I tried:
/dts-v1/;
/ {
        binman: binman {
                multiple-images;
            my_template: template {
                    blob-ext@0 {
                            filename = "my-blob.bin";
                            offset = <0>;
                    };
                    blob-ext@100 {
                            filename = "/dev/null";
                            offset = <0x100>;
                    };
            };

            my-image {
                    filename = "my-image.bin";
                    insert-template = <&my_template>;
                    blob-ext@100 {
                            filename = "my-blob2.bin";
                    };
            };
    };

};
First, I had to apply that null filename workaround, and the overridden
filename is taken for the final image (if I leave out blob-ext@0), but
this is at least ugly.
However, the file above as-is does not build:
binman: Node '/binman/my-image/blob-ext@0': Offset 0x0 (0) overlaps with
previous entry '/binman/my-image/blob-ext@100' ending at 0x107 (263)
Probably for the same reason, leaving fit,fdt-list-val out in a template
also generates an error during build. Putting an empty string there and
overriding it later does not work.
I think it is just that it doesn't support templating of
multiple-images nodes property. I sent a patch for this [1]
There may be other cases too, but if you have a failure, please see if
you can adjust that test (287) to fail for you.
Hmm, already the test pattern from [1] still gives me
binman: Node '/binman/image/blob-ext@0': Offset 0x0 (0) overlaps with
previous entry '/binman/image/blob-ext@8' ending at 0xf (15)
I've applied this series and then [1] on top.
Can you please list the commits so I can check it? I have:
88a31cccc20 (HEAD -> mkim2, dm/mkim-working) binman: Support
templating with multiple images
71f1ef46d0c (dm-public/mkim-working) binman: Support simple templates
8841829b4c8 dtoc: Allow inserting a list of nodes into another
fd2fb91c616 dtoc: Support copying the contents of a node into another
288f9e7c3b6 binman: Correct handling of zero bss size
4725a43d0ef binman: Drop __bss_size variable in bss_data.c
c4bfe4917c9 binman: Provide a way to specific the fdt-list directly
6e571e4d260 binman: Convert mkimage to Entry_section
5fd2892283f stm32mp15: Avoid writing symbols in SPL
24c60031f48 binman: Allow disabling symbol writing
5c6a660333c binman: Read _multiple_data_files in the correct place
24a273113bf binman: Use GetEntries() to obtain section contents
1e0659321d8 binman: Init align_default in entry_Section
cc3699d1d04 binman: Correct coverage gap in control
67d8b46e6ef (us/WIP/28Jun2023-next) Merge tag
'u-boot-amlogic-next-20230628' of
https://source.denx.de/u-boot/custodians/u-boot-amlogic into next
I was using the dtoc patches from this series, not the updated but
apparently not yet sent ones from your branch. Those seem to make a
difference. Continuing to test.
I thought I did send out v2. There are some test fails with large
numbers of CPUs which I need to fix in v3, but anyway, please use the
branch I pushed for testing, and sorry for any discrepancy.
Doing this now. And here is the next issue - maybe the last:
/dts-v1/;
/ {
    binman: binman {
    	multiple-images;
my_template: template {
    		fit@0 {
    			images {
    				kernel-1 {
    				};
    				kernel-2 {
    				};
    			};
    		};
    	};
image {
    		filename = "my-image.bin";
    		insert-template = <&my_template>;
fit@0 {
    			images {
    				kernel-3 {
    				};
    			};
    		};
    	};
    };
};
Gives
binman: pylibfdt error -4: FDT_ERR_BADOFFSET
when trying to build it. That subnode kernel-3 can't be added.
Jan
-- 
Siemens AG, Technology
Competence Center Embedded Linux