[v3,06/11] binman: capsule: Add support for generating capsules

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@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

Hi Sughosh,

On Sun, 9 Jul 2023 at 07:34, Sughosh Ganu <sughosh.ganu@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@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.

>
> 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

hi Simon,

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

Hi Sughosh,

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

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