diff mbox series

[3/8] binman: capsule: Generate capsules through config file

Message ID 20230908120002.29851-4-sughosh.ganu@linaro.org
State New
Headers show
Series Add some more EFI capsule tooling support | expand

Commit Message

Sughosh Ganu Sept. 8, 2023, 11:59 a.m. UTC 
Add support in binman for generating capsules by reading the capsule
parameters through a config file. Also add a test case in binman for
this mode of capsule generation.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---

 tools/binman/entries.rst                   | 35 ++++++++++++
 tools/binman/etype/efi_capsule_cfg_file.py | 66 ++++++++++++++++++++++
 tools/binman/ftest.py                      | 29 ++++++++++
 tools/binman/test/319_capsule_cfg.dts      | 15 +++++
 4 files changed, 145 insertions(+)
 create mode 100644 tools/binman/etype/efi_capsule_cfg_file.py
 create mode 100644 tools/binman/test/319_capsule_cfg.dts

Comments

Simon Glass Sept. 10, 2023, 7:13 p.m. UTC | #1 
Hi Sughosh,

On Fri, 8 Sept 2023 at 06:00, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> Add support in binman for generating capsules by reading the capsule
> parameters through a config file. Also add a test case in binman for
> this mode of capsule generation.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>
>  tools/binman/entries.rst                   | 35 ++++++++++++
>  tools/binman/etype/efi_capsule_cfg_file.py | 66 ++++++++++++++++++++++
>  tools/binman/ftest.py                      | 29 ++++++++++
>  tools/binman/test/319_capsule_cfg.dts      | 15 +++++
>  4 files changed, 145 insertions(+)
>  create mode 100644 tools/binman/etype/efi_capsule_cfg_file.py
>  create mode 100644 tools/binman/test/319_capsule_cfg.dts

I think we discussed this some time ago. The problem I have with this
is that it adds another way of specifying the image description,
separate from the binman definition. This introduces all sorts of
problems:

- the question of where the filenames are located, something which
binman tries to handle cohesively
- binman has no knowledge of what is being put in the capsules
- the resulting fdtmap lacks any information about what is in the capsules
- it seems to create two ways of doing the same thing

Binman can presumably already create multiple capsule files, just by
adding to the description.

What need does this feature meet?

Regards,
Simon
Sughosh Ganu Sept. 11, 2023, 2:13 p.m. UTC | #2 
hi Simon,

On Mon, 11 Sept 2023 at 00:44, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Fri, 8 Sept 2023 at 06:00, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > Add support in binman for generating capsules by reading the capsule
> > parameters through a config file. Also add a test case in binman for
> > this mode of capsule generation.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >
> >  tools/binman/entries.rst                   | 35 ++++++++++++
> >  tools/binman/etype/efi_capsule_cfg_file.py | 66 ++++++++++++++++++++++
> >  tools/binman/ftest.py                      | 29 ++++++++++
> >  tools/binman/test/319_capsule_cfg.dts      | 15 +++++
> >  4 files changed, 145 insertions(+)
> >  create mode 100644 tools/binman/etype/efi_capsule_cfg_file.py
> >  create mode 100644 tools/binman/test/319_capsule_cfg.dts
>
> I think we discussed this some time ago. The problem I have with this
> is that it adds another way of specifying the image description,
> separate from the binman definition. This introduces all sorts of
> problems:
>
> - the question of where the filenames are located, something which
> binman tries to handle cohesively
> - binman has no knowledge of what is being put in the capsules
> - the resulting fdtmap lacks any information about what is in the capsules
> - it seems to create two ways of doing the same thing
>
> Binman can presumably already create multiple capsule files, just by
> adding to the description.
>
> What need does this feature meet?

We already had this discussion earlier about the need for generating
capsules through the config file, where I had explained the need for
this [1]. This is functionality which is supported by the
specification, and we have folks who are interested in having the
capsules generated through config file, and also in generation of a
single capsule file consisting of multiple input payloads.

I understand that this model does not fit with the usual way in binman
of generating an output image by specifying it's inputs as DT nodes.
But just because this does not fit or align with the design of the
tool does not mean that we do not add support for the functionality.
Why can't we support this in binman as long as it is properly
documented as to what is being done. Moreover, my initial versions of
this patchset were attempting to achieve this by adding a make target,
but you had shot it down [2]. So it either has to be done through
binman, or through a make target.

-sughosh

[1] - https://lists.denx.de/pipermail/u-boot/2023-July/524849.html
[2] - https://lists.denx.de/pipermail/u-boot/2023-June/521103.html
Simon Glass Sept. 13, 2023, 6:07 p.m. UTC | #3 
Hi Sughosh,

On Mon, 11 Sept 2023 at 08:13, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Mon, 11 Sept 2023 at 00:44, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Fri, 8 Sept 2023 at 06:00, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > Add support in binman for generating capsules by reading the capsule
> > > parameters through a config file. Also add a test case in binman for
> > > this mode of capsule generation.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > >
> > >  tools/binman/entries.rst                   | 35 ++++++++++++
> > >  tools/binman/etype/efi_capsule_cfg_file.py | 66 ++++++++++++++++++++++
> > >  tools/binman/ftest.py                      | 29 ++++++++++
> > >  tools/binman/test/319_capsule_cfg.dts      | 15 +++++
> > >  4 files changed, 145 insertions(+)
> > >  create mode 100644 tools/binman/etype/efi_capsule_cfg_file.py
> > >  create mode 100644 tools/binman/test/319_capsule_cfg.dts
> >
> > I think we discussed this some time ago. The problem I have with this
> > is that it adds another way of specifying the image description,
> > separate from the binman definition. This introduces all sorts of
> > problems:
> >
> > - the question of where the filenames are located, something which
> > binman tries to handle cohesively
> > - binman has no knowledge of what is being put in the capsules
> > - the resulting fdtmap lacks any information about what is in the capsules
> > - it seems to create two ways of doing the same thing
> >
> > Binman can presumably already create multiple capsule files, just by
> > adding to the description.
> >
> > What need does this feature meet?
>
> We already had this discussion earlier about the need for generating
> capsules through the config file, where I had explained the need for
> this [1]. This is functionality which is supported by the
> specification, and we have folks who are interested in having the
> capsules generated through config file, and also in generation of a
> single capsule file consisting of multiple input payloads.
>
> I understand that this model does not fit with the usual way in binman
> of generating an output image by specifying it's inputs as DT nodes.
> But just because this does not fit or align with the design of the
> tool does not mean that we do not add support for the functionality.
> Why can't we support this in binman as long as it is properly
> documented as to what is being done. Moreover, my initial versions of
> this patchset were attempting to achieve this by adding a make target,
> but you had shot it down [2]. So it either has to be done through
> binman, or through a make target.

We have a lot of balls in the air right now...I would like to pause
this one until we get the other things sorted out:

- simple capsule update in sandbox (see my other email)
- example of how to use all this on a real board (Tom is going to make
a proposal there)
- tidy up the testing so that it is easier to understand and fits
better with the test framework
- figure out the DT story, since we are still struggling to align
there (Binman bindings, removing nodes, etc)

We don't need to get all those done, but we do need to at least agree
on the direction.

Regards,
Simon


>
> -sughosh
>
> [1] - https://lists.denx.de/pipermail/u-boot/2023-July/524849.html
> [2] - https://lists.denx.de/pipermail/u-boot/2023-June/521103.html
Sughosh Ganu Sept. 14, 2023, 11:41 a.m. UTC | #4 
hi Simon,

On Wed, 13 Sept 2023 at 23:37, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Mon, 11 Sept 2023 at 08:13, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Mon, 11 Sept 2023 at 00:44, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Fri, 8 Sept 2023 at 06:00, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > Add support in binman for generating capsules by reading the capsule
> > > > parameters through a config file. Also add a test case in binman for
> > > > this mode of capsule generation.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > ---
> > > >
> > > >  tools/binman/entries.rst                   | 35 ++++++++++++
> > > >  tools/binman/etype/efi_capsule_cfg_file.py | 66 ++++++++++++++++++++++
> > > >  tools/binman/ftest.py                      | 29 ++++++++++
> > > >  tools/binman/test/319_capsule_cfg.dts      | 15 +++++
> > > >  4 files changed, 145 insertions(+)
> > > >  create mode 100644 tools/binman/etype/efi_capsule_cfg_file.py
> > > >  create mode 100644 tools/binman/test/319_capsule_cfg.dts
> > >
> > > I think we discussed this some time ago. The problem I have with this
> > > is that it adds another way of specifying the image description,
> > > separate from the binman definition. This introduces all sorts of
> > > problems:
> > >
> > > - the question of where the filenames are located, something which
> > > binman tries to handle cohesively
> > > - binman has no knowledge of what is being put in the capsules
> > > - the resulting fdtmap lacks any information about what is in the capsules
> > > - it seems to create two ways of doing the same thing
> > >
> > > Binman can presumably already create multiple capsule files, just by
> > > adding to the description.
> > >
> > > What need does this feature meet?
> >
> > We already had this discussion earlier about the need for generating
> > capsules through the config file, where I had explained the need for
> > this [1]. This is functionality which is supported by the
> > specification, and we have folks who are interested in having the
> > capsules generated through config file, and also in generation of a
> > single capsule file consisting of multiple input payloads.
> >
> > I understand that this model does not fit with the usual way in binman
> > of generating an output image by specifying it's inputs as DT nodes.
> > But just because this does not fit or align with the design of the
> > tool does not mean that we do not add support for the functionality.
> > Why can't we support this in binman as long as it is properly
> > documented as to what is being done. Moreover, my initial versions of
> > this patchset were attempting to achieve this by adding a make target,
> > but you had shot it down [2]. So it either has to be done through
> > binman, or through a make target.
>
> We have a lot of balls in the air right now...I would like to pause
> this one until we get the other things sorted out:

Okay, fair enough. I will check if the capsule generation through
config file can be added to the mkeficapsule tool in the meantime.
Also, can you review the last two patches of this series [1] [2] which
are adding support for empty capsules. I don't think they are at odds
with the idea of binman. Thanks.

-sughosh

[1] - https://patchwork.ozlabs.org/project/uboot/patch/20230908120002.29851-8-sughosh.ganu@linaro.org/
[2] - https://patchwork.ozlabs.org/project/uboot/patch/20230908120002.29851-9-sughosh.ganu@linaro.org/

>
> - simple capsule update in sandbox (see my other email)
> - example of how to use all this on a real board (Tom is going to make
> a proposal there)
> - tidy up the testing so that it is easier to understand and fits
> better with the test framework
> - figure out the DT story, since we are still struggling to align
> there (Binman bindings, removing nodes, etc)
>
> We don't need to get all those done, but we do need to at least agree
> on the direction.
>
> Regards,
> Simon
>
>
> >
> > -sughosh
> >
> > [1] - https://lists.denx.de/pipermail/u-boot/2023-July/524849.html
> > [2] - https://lists.denx.de/pipermail/u-boot/2023-June/521103.html
diff mbox series

Patch

diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
index 801bd94674..a68ea2cb21 100644
--- a/tools/binman/entries.rst
+++ b/tools/binman/entries.rst
@@ -532,6 +532,41 @@  payload using the blob-ext subnode.
 
 
 
+.. _etype_efi_capsule_cfg_file:
+
+Entry: capsule: Entry for generating EFI Capsule files through config file
+--------------------------------------------------------------------------
+
+This is an entry for generating EFI capsules through a config
+file. The parameters needed for generation of the capsules are
+provided through a config file. This results in generation of one or
+multiple capsules, corresponding to the entries in the config file.
+
+Properties / Entry arguments:
+    - cfg-file: Config file for providing capsule parameters. These are
+      parameters needed for generating the capsules. The parameters can
+      be listed by running the './tools/mkeficapsule -h' command.
+
+For more details on the description of the capsule format, and the capsule
+update functionality, refer Section 8.5 and Chapter 23 in the `UEFI
+specification`_.
+
+A typical capsule entry node would then look something like this::
+
+    capsule {
+            type = "efi-capsule-cfg-file";
+            cfg-file = "path/to/the/config/file";
+    };
+
+In the above example, the entry only contains the path to the config file.
+All parameters needed for generation of the capsule, including the input
+payload image and the output capsule file are specified through the entries
+in the config file.
+
+.. _`UEFI specification`: https://uefi.org/sites/default/files/resources/UEFI_Spec_2_10_Aug29.pdf
+
+
+
 .. _etype_encrypted:
 
 Entry: encrypted: Externally built encrypted binary blob
diff --git a/tools/binman/etype/efi_capsule_cfg_file.py b/tools/binman/etype/efi_capsule_cfg_file.py
new file mode 100644
index 0000000000..ccf27077ec
--- /dev/null
+++ b/tools/binman/etype/efi_capsule_cfg_file.py
@@ -0,0 +1,66 @@ 
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (c) 2023 Linaro Limited
+#
+# Entry-type module for producing a EFI capsule through a
+# config file.
+#
+
+import os
+
+from binman.entry import Entry
+from dtoc import fdt_util
+from u_boot_pylib import tools
+
+class Entry_efi_capsule_cfg_file(Entry):
+    """Entry for generating EFI capsules through config file
+
+    This is an entry for generating EFI capsules through a
+    config file.
+
+    The parameters needed for generation of the capsules are
+    provided through a config file. This results in generation
+    of one or multiple capsules, corresponding to the entries
+    in the config file.
+
+    Properties / Entry arguments:
+    - cfg-file: Config file for providing capsule parameters. These are
+      parameters needed for generating the capsules. The parameters can
+      be listed by running the './tools/mkeficapsule -h' command.
+
+    For more details on the description of the capsule format, and the capsule
+    update functionality, refer Section 8.5 and Chapter 23 in the `UEFI
+    specification`_.
+
+    A typical capsule entry node would then look something like this
+
+    capsule {
+            type = "efi-capsule-cfg-file";
+            cfg-file = "path/to/the/config/file";
+    };
+
+    In the above example, the entry only contains the path to the config file.
+    All parameters needed for generation of the capsule, including the input
+    payload image and the output capsule file are specified through the entries
+    in the config file.
+
+    .. _`UEFI specification`: https://uefi.org/sites/default/files/resources/UEFI_Spec_2_10_Aug29.pdf
+"""
+    def __init__(self, section, etype, node):
+        super().__init__(section, etype, node)
+        self.required_props = ['cfg-file']
+
+    def ReadNode(self):
+        super().ReadNode()
+
+        self.cfg_file = fdt_util.GetString(self._node, 'cfg-file')
+        if not os.path.isabs(self.cfg_file):
+            self.cfg_file = tools.get_input_filename(self.cfg_file)
+
+    def _GenCapsule(self):
+        self.mkeficapsule.generate_capsule_cfg_file(self.cfg_file)
+
+    def ObtainContents(self):
+        self._GenCapsule()
+
+    def AddBintools(self, btools):
+        self.mkeficapsule = self.AddBintool(btools, 'mkeficapsule')
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 8e419645a6..654af2c617 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -7334,5 +7334,34 @@  fdt         fdtmap                Extract the devicetree blob from the fdtmap
         self.assertIn("entry is missing properties: image-guid",
                       str(e.exception))
 
+    def _SetupTmpOutDir(self, outfname):
+        self.tmpdir = tempfile.mkdtemp(prefix='binman.')
+        self.capsule_fname = os.path.join(self.tmpdir, outfname)
+
+    def _BuildCapsuleCfgFile(self):
+        cfg_file = self._MakeInputFile('capsule_cfg_file.txt', b'')
+        payload_fname = self._MakeInputFile('capsule_input.bin', EFI_CAPSULE_DATA)
+        self._SetupTmpOutDir('image.bin')
+
+        with open(f'{cfg_file}', 'w') as fd:
+            fd.write('{\n')
+            fd.write('\timage-index: 0x1\n')
+            fd.write('\timage-guid: 09d7cf52-0720-4710-91d1-08469b7fe9c8\n')
+            fd.write(f'\tpayload: {payload_fname}\n')
+            fd.write(f'\tcapsule: {self.capsule_fname}\n')
+            fd.write('}\n')
+
+    def testCapsuleGenCfgFile(self):
+        """Test generation of EFI capsule through config file"""
+        self._BuildCapsuleCfgFile()
+
+        self._DoReadFile('319_capsule_cfg.dts')
+
+        data = tools.read_file(self.capsule_fname)
+        self._CheckCapsule(data)
+
+        if not self.preserve_outdirs:
+            shutil.rmtree(self.tmpdir)
+
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/test/319_capsule_cfg.dts b/tools/binman/test/319_capsule_cfg.dts
new file mode 100644
index 0000000000..3e07bdd962
--- /dev/null
+++ b/tools/binman/test/319_capsule_cfg.dts
@@ -0,0 +1,15 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		efi-capsule {
+			type = "efi-capsule-cfg-file";
+			cfg-file = "capsule_cfg_file.txt";
+		};
+	};
+};