Re: [PATCH v3 06/11] binman: capsule: Add support for generating capsules
Hi Sughosh, On Tue, 11 Jul 2023 at 01:13, Sughosh Ganu wrote: > > hi Simon, > > On Tue, 11 Jul 2023 at 03:08, Simon Glass wrote: > > > > Hi Sughosh, > > > > On Sun, 9 Jul 2023 at 07:34, Sughosh Ganu 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 > > > --- > > > 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. The binman tests are for binman, which is a self-contained program with 100% test coverage. That is a very important feature, since lots of random SoC-specific features depend on it. If something breaks, it can be a bit of a disaster. Also, 100% test coverage allows refactoring of binman's code without worry about breaking something. This is also very important, since we will find problems which the current code base cannot solve. People need the confidence to refactor In practice, the tests define the behaviour. Code which is not tested is therefore considered 'dead' code and should be deleted. A notable point with binman tests is that all failure modes are tested, not just 'happy path'. [..] Regards, Simon
Re: [PATCH v3 06/11] binman: capsule: Add support for generating capsules
hi Simon,
On Tue, 11 Jul 2023 at 03:08, Simon Glass wrote:
>
> Hi Sughosh,
>
> On Sun, 9 Jul 2023 at 07:34, Sughosh Ganu 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
> > ---
> > 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 00..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]
> > +Options:
> > + -g, --guid guid for image blob type
> > + -i, --index update image index
> > + -I, --instanceupdate hardware instance
> > + -p, --private-key private key file
> > + -c, --certificate signer's certificate file
> > + -m, --monotonic-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 0x and
> > 0x
> > + -f, --cfg-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')
> > +
Re: [PATCH v3 06/11] binman: capsule: Add support for generating capsules
Hi Sughosh,
On Sun, 9 Jul 2023 at 07:34, Sughosh Ganu 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
> ---
> 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.
>
> diff --git a/tools/binman/btool/mkeficapsule.py
> b/tools/binman/btool/mkeficapsule.py
> new file mode 100644
> index 00..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]
> +Options:
> + -g, --guid guid for image blob type
> + -i, --index update image index
> + -I, --instanceupdate hardware instance
> + -p, --private-key private key file
> + -c, --certificate signer's certificate file
> + -m, --monotonic-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 0x and
> 0x
> + -f, --cfg-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
m
