[PATCH v3 06/11] binman: capsule: Add support for generating capsules
Sughosh Ganu
sughosh.ganu at linaro.org
Tue Jul 11 09:13:10 CEST 2023
hi Simon,
On Tue, 11 Jul 2023 at 03:08, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Sughosh,
>
> On Sun, 9 Jul 2023 at 07:34, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >
> > Add support in binman for generating capsules. The capsule parameters
> > can be specified either through a config file or through the capsule
> > binman entry.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > ---
> > Changes since V2:
> > * New patch which generates capsules through binman replacing the
> > earlier make target.
> >
> > tools/binman/btool/mkeficapsule.py | 91 +++++++++++++++++++++++++
> > tools/binman/entries.rst | 27 ++++++++
> > tools/binman/etype/capsule.py | 102 +++++++++++++++++++++++++++++
> > 3 files changed, 220 insertions(+)
> > create mode 100644 tools/binman/btool/mkeficapsule.py
> > create mode 100644 tools/binman/etype/capsule.py
>
> Please do check test coverage (binman test -T). You are missing quite
> a lot in two two files you have added.
I was aware of adding tests in binman, but since the capsules
generated through binman are getting tested in the capsule update
functionality, I thought this would be superfluous. If this is
mandatory, I will add the tests. Will also address the rest of your
comments for this patch.
-sughosh
>
> >
> > diff --git a/tools/binman/btool/mkeficapsule.py b/tools/binman/btool/mkeficapsule.py
> > new file mode 100644
> > index 0000000000..9f656c12cf
> > --- /dev/null
> > +++ b/tools/binman/btool/mkeficapsule.py
> > @@ -0,0 +1,91 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> > +# Copyright 2023 Linaro Limited
> > +#
> > +"""Bintool implementation for mkeficapsule tool
> > +
> > +mkeficapsule is a tool used for generating EFI capsules.
> > +
> > +The following are the command-line options to be provided
> > +to the tool
> > +Usage: mkeficapsule [options] <image blob> <output file>
> > +Options:
> > + -g, --guid <guid string> guid for image blob type
> > + -i, --index <index> update image index
> > + -I, --instance <instance> update hardware instance
> > + -p, --private-key <privkey file> private key file
> > + -c, --certificate <cert file> signer's certificate file
> > + -m, --monotonic-count <count> monotonic count
> > + -d, --dump_sig dump signature (*.p7)
> > + -A, --fw-accept firmware accept capsule, requires GUID, no image blob
> > + -R, --fw-revert firmware revert capsule, takes no GUID, no image blob
> > + -o, --capoemflag Capsule OEM Flag, an integer between 0x0000 and 0xffff
> > + -f, --cfg-file <config file> config file with capsule parameters
> > + -h, --help print a help message
> > +
> > +"""
> > +
> > +from binman import bintool
> > +
> > +class Bintoolmkeficapsule(bintool.Bintool):
> > + """Handles the 'mkeficapsule' tool
> > +
> > + This bintool is used for generating the EFI capsules. The
> > + capsule generation parameters can either be specified through
> > + command-line, or through a config file.
> > +
> > + """
> > + def __init__(self, name):
> > + super().__init__(name, 'mkeficapsule tool for generating capsules')
> > +
> > + def capsule_cfg_file(self, cfg_file):
>
> """Function comment
>
> Args:
> cfg_file (str): ...
> """
>
> Please fix throughout
>
> > +
> > + args = [
> > + f'--cfg-file={cfg_file}'
> > + ]
> > + self.run_cmd(*args)
> > +
> > + def cmdline_capsule(self, image_index, image_guid, hardware_instance,
> > + payload, output_fname):
> > +
> > + args = [
> > + f'--index={image_index}',
> > + f'--guid={image_guid}',
> > + f'--instance={hardware_instance}',
> > + payload,
> > + output_fname
> > + ]
> > + self.run_cmd(*args)
> > +
> > + def cmdline_auth_capsule(self, image_index, image_guid, hardware_instance,
> > + monotonic_count, priv_key, pub_key,
> > + payload, output_fname):
> > +
> > + args = [
> > + f'--index={image_index}',
> > + f'--guid={image_guid}',
> > + f'--instance={hardware_instance}',
> > + f'--monotonic-count={monotonic_count}',
> > + f'--private-key={priv_key}',
> > + f'--certificate={pub_key}',
> > + payload,
> > + output_fname
> > + ]
> > + self.run_cmd(*args)
> > +
> > + def fetch(self, method):
> > + """Fetch handler for mkeficapsule
> > +
> > + This builds the tool from source
> > +
> > + Returns:
> > + tuple:
> > + str: Filename of fetched file to copy to a suitable directory
> > + str: Name of temp directory to remove, or None
> > + """
> > + if method != bintool.FETCH_BUILD:
> > + return None
> > + result = self.build_from_git(
> > + 'https://source.denx.de/u-boot/u-boot.git',
> > + 'tools',
> > + 'tools/mkeficapsule')
> > + return result
>
> Can we just install u-boot-tools? But if not, this is OK.
>
> But it does not actually work:
>
> $ binman tool -f mkeficapsule
> Fetch: mkeficapsule
> - trying method: binary download
> - trying method: build from source
> - clone git repo 'https://source.denx.de/u-boot/u-boot.git' to
> '/tmp/binmanf.chfbsxc0'
> - build target 'tools'
> Exception: Error 2 running 'make -C /tmp/binmanf.chfbsxc0 -j 64 tools': ***
> *** Configuration file ".config" not found!
> ***
> *** Please run some configurator (e.g. "make oldconfig" or
> *** "make menuconfig" or "make xconfig").
> ***
> make[2]: *** [scripts/kconfig/Makefile:75: syncconfig] Error 1
> make[1]: *** [Makefile:576: syncconfig] Error 2
> make: *** No rule to make target 'include/config/auto.conf', needed by
> 'include/config/uboot.release'. Stop.
>
> - failed to fetch with all methods
>
> I think you need a make tools_defconfig in there.
>
> > diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
> > index b71af801fd..9a263e8691 100644
> > --- a/tools/binman/entries.rst
> > +++ b/tools/binman/entries.rst
> > @@ -283,6 +283,33 @@ entry; similarly for SPL.
> >
> >
> >
> > +.. _etype_capsule:
> > +
> > +Entry: capsule: Entry for generating EFI Capsule files
> > +------------------------------------------------------
> > +
> > +This is an entry for generating EFI capsules.
> > +
> > +The parameters needed for generation of the capsules can either be
> > +provided separately, or through a config file.
> > +
> > +Properties / Entry arguments:
>
> Please add more detail to answer below questions.
>
> > + - cfg-file: Config file for providing capsule
> > + parameters.
>
> What parameters? Please provide a link
>
> > + - image-index: Unique number for identifying
> > + corresponding payload image.
>
> Is this indexed from 0 ?
>
> > + - image-type-id: Image GUID which will be used
> > + for identifying the image.
>
> In what way is it identifying it? Is it identifying the board that the
> image is for, or something else?
>
> > + - hardware-instance: Optional number for identifying
> > + unique hardware instance of a device in the system.
>
> Is this numbered from 0?
>
> > + - monotomic-count: Count used when signing an image.
>
> monotonic
>
> What count? What is it used for?
>
> > + - private-key: Path to private key file.
>
> File format?
>
> > + - pub-key-cert: Path to public key certificate file.
>
> File format?
>
> > + - filename: Path to the input(payload) file.
>
> This is just a binary file to be signed, right?
>
> > + - capsule: Path to the output capsule file.
>
> File format?
>
> > +
> > +
> > +
> > .. _etype_cbfs:
> >
> > Entry: cbfs: Coreboot Filesystem (CBFS)
> > diff --git a/tools/binman/etype/capsule.py b/tools/binman/etype/capsule.py
> > new file mode 100644
> > index 0000000000..a5ada885a6
> > --- /dev/null
> > +++ b/tools/binman/etype/capsule.py
> > @@ -0,0 +1,102 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> > +# Copyright (c) 2023 Linaro Limited
> > +#
> > +# Entry-type module for producing a capsule
> > +#
> > +
> > +import os
>
> blank line here (I think it is the convention to put a blank line
> after OS includes)
>
> > +from u_boot_pylib import tools
> > +from dtoc import fdt_util
> > +from binman.entry import Entry
>
> We should order these in reverse. I suspect that the conversion I did
> from patman to u_boot_pylib was not correct, as I probably forgot to
> fix the order.
>
> > +
> > +class Entry_capsule(Entry):
> > + """Entry for generating EFI capsules
> > +
> > + This is an entry for generating EFI capsules.
> > +
> > + The parameters needed for generation of the capsules can
> > + either be provided separately, or through a config file.
> > +
> > + Properties / Entry arguments:
> > + - cfg-file: Config file for providing capsule
> > + parameters.
> > + - image-index: Unique number for identifying
> > + corresponding payload image.
> > + - image-type-id: Image GUID which will be used
> > + for identifying the image.
> > + - hardware-instance: Optional number for identifying
> > + unique hardware instance of a device in the system.
> > + - monotomic-count: Count used when signing an image.
> > + - private-key: Path to private key file.
> > + - pub-key-cert: Path to public key certificate file.
> > + - filename: Path to the input(payload) file.
> > + - capsule: Path to the output capsule file.
>
> copy docs from above to here, when updated
>
> > +
> > + """
> > + def __init__(self, section, etype, node):
> > + super().__init__(section, etype, node)
> > + self.image_index = 0
> > + self.image_guid = ""
> > + self.hardware_instance = 0
> > + self.monotonic_count = 0
> > + self.private_key = ""
> > + self.pub_key_cert = ""
> > + self.auth = 0
> > + self.payload = ""
> > + self.capsule_fname = ""
> > +
> > + def ReadNode(self):
> > + super().ReadNode()
> > +
> > + self.cfg_file = fdt_util.GetString(self._node, 'cfg-file')
> > + if not self.cfg_file:
> > + self.image_index = fdt_util.GetInt(self._node, 'image-index')
> > + if not self.image_index:
> > + self.Raise('mkeficapsule must be provided an Image Index')
> > +
> > + self.image_guid = fdt_util.GetString(self._node, 'image-type-id')
> > + if not self.image_guid:
> > + self.Raise('mkeficapsule must be provided an Image GUID')
>
> You can handle this with something like this in your __init__():
>
> self.required_props = ['image-index', 'image-type-id', ...]
>
> Sadly the docs for required_props are wrong.
>
> > +
> > + self.hardware_instance = fdt_util.GetInt(self._node, 'hardware-instance')
> > + self.monotonic_count = fdt_util.GetInt(self._node, 'monotonic-count')
> > +
> > + self.private_key = fdt_util.GetString(self._node, 'private-key')
> > + self.pub_key_cert = fdt_util.GetString(self._node, 'pub-key-cert')
> > +
> > + if ((self.private_key and not self.pub_key_cert) or (self.pub_key_cert and not self.private_key)):
> > + self.Raise('Both private-key and public key Certificate need to be provided')
> > + elif not (self.private_key and self.pub_key_cert):
> > + self.auth = 0
> > + else:
> > + self.auth = 1
> > +
> > + self.payload = fdt_util.GetString(self._node, 'filename')
> > + if not self.payload:
> > + self.Raise('mkeficapsule must be provided an input filename(payload)')
> > +
> > + self.capsule_fname = fdt_util.GetString(self._node, 'capsule')
> > + if not self.capsule_fname:
> > + self.capsule_fname = os.path.splitext(self.payload)[0] + '.capsule'
> > +
> > + def ObtainContents(self):
> > + if self.cfg_file:
> > + self.mkeficapsule.capsule_cfg_file(self.cfg_file)
> > + elif self.auth:
> > + self.mkeficapsule.cmdline_auth_capsule(self.image_index,
> > + self.image_guid,
> > + self.hardware_instance,
> > + self.monotonic_count,
> > + self.private_key,
> > + self.pub_key_cert,
> > + self.payload,
> > + self.capsule_fname)
> > + else:
> > + self.mkeficapsule.cmdline_capsule(self.image_index,
> > + self.image_guid,
> > + self.hardware_instance,
> > + self.payload,
> > + self.capsule_fname)
>
> This does not actually obtain the contents. I think you need:
>
> self.SetContents(...)
>
> here.
>
> > +
> > + def AddBintools(self, btools):
> > + self.mkeficapsule = self.AddBintool(btools, 'mkeficapsule')
> > --
> > 2.34.1
> >
>
> Regards,
> Simon
More information about the U-Boot
mailing list