hi Simon, On Tue, 11 Jul 2023 at 03:08, Simon Glass <s...@chromium.org> wrote: > > Hi Sughosh, > > On Sun, 9 Jul 2023 at 07:34, Sughosh Ganu <sughosh.g...@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.g...@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