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

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