diff mbox series

[v7,08/11] binman: capsule: Add support for generating EFI capsules

Message ID 20230805113458.1430239-9-sughosh.ganu@linaro.org
State New
Headers show
Series Integrate EFI capsule tasks into u-boot's build flow | expand

Commit Message

Sughosh Ganu Aug. 5, 2023, 11:34 a.m. UTC 
Add support in binman for generating EFI capsules. The capsule
parameters can be specified through the capsule binman entry. Also add
test cases in binman for testing capsule generation.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
Changes since V6:
* Add macros for the GUID strings in sandbox_efi_capsule.h
* Highlight that the private and public keys are mandatory for capsule
  signing.
* Add a URL link to the UEFI spec, as used in the rst files.
* Use local vars for private and public keys in BuildSectionData()
* Use local vars for input payload and capsule filenames in
  BuildSectionData().
* Drop the ProcessContents() and SetImagePos() as the superclass
  functions suffice.
* Use GUID macro names in the capsule test dts files.
* Rename efi_capsule_payload.bin to capsule_input.bin.


 include/sandbox_efi_capsule.h                 |  14 ++
 tools/binman/entries.rst                      |  62 ++++++++
 tools/binman/etype/efi_capsule.py             | 143 ++++++++++++++++++
 tools/binman/ftest.py                         | 121 +++++++++++++++
 tools/binman/test/307_capsule.dts             |  23 +++
 tools/binman/test/308_capsule_signed.dts      |  25 +++
 tools/binman/test/309_capsule_version.dts     |  24 +++
 tools/binman/test/310_capsule_signed_ver.dts  |  26 ++++
 tools/binman/test/311_capsule_oemflags.dts    |  24 +++
 tools/binman/test/312_capsule_missing_key.dts |  24 +++
 .../binman/test/313_capsule_missing_index.dts |  22 +++
 .../binman/test/314_capsule_missing_guid.dts  |  19 +++
 .../test/315_capsule_missing_payload.dts      |  19 +++
 13 files changed, 546 insertions(+)
 create mode 100644 include/sandbox_efi_capsule.h
 create mode 100644 tools/binman/etype/efi_capsule.py
 create mode 100644 tools/binman/test/307_capsule.dts
 create mode 100644 tools/binman/test/308_capsule_signed.dts
 create mode 100644 tools/binman/test/309_capsule_version.dts
 create mode 100644 tools/binman/test/310_capsule_signed_ver.dts
 create mode 100644 tools/binman/test/311_capsule_oemflags.dts
 create mode 100644 tools/binman/test/312_capsule_missing_key.dts
 create mode 100644 tools/binman/test/313_capsule_missing_index.dts
 create mode 100644 tools/binman/test/314_capsule_missing_guid.dts
 create mode 100644 tools/binman/test/315_capsule_missing_payload.dts

Comments

Simon Glass Aug. 5, 2023, 3:03 p.m. UTC | #1 
Hi Sughosh,

On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> Add support in binman for generating EFI capsules. The capsule
> parameters can be specified through the capsule binman entry. Also add
> test cases in binman for testing capsule generation.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
> Changes since V6:
> * Add macros for the GUID strings in sandbox_efi_capsule.h
> * Highlight that the private and public keys are mandatory for capsule
>   signing.
> * Add a URL link to the UEFI spec, as used in the rst files.
> * Use local vars for private and public keys in BuildSectionData()
> * Use local vars for input payload and capsule filenames in
>   BuildSectionData().
> * Drop the ProcessContents() and SetImagePos() as the superclass
>   functions suffice.
> * Use GUID macro names in the capsule test dts files.
> * Rename efi_capsule_payload.bin to capsule_input.bin.
>
>
>  include/sandbox_efi_capsule.h                 |  14 ++

Please move this file to a later patch - see below.

Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.

>  tools/binman/entries.rst                      |  62 ++++++++
>  tools/binman/etype/efi_capsule.py             | 143 ++++++++++++++++++
>  tools/binman/ftest.py                         | 121 +++++++++++++++
>  tools/binman/test/307_capsule.dts             |  23 +++
>  tools/binman/test/308_capsule_signed.dts      |  25 +++
>  tools/binman/test/309_capsule_version.dts     |  24 +++
>  tools/binman/test/310_capsule_signed_ver.dts  |  26 ++++
>  tools/binman/test/311_capsule_oemflags.dts    |  24 +++
>  tools/binman/test/312_capsule_missing_key.dts |  24 +++
>  .../binman/test/313_capsule_missing_index.dts |  22 +++
>  .../binman/test/314_capsule_missing_guid.dts  |  19 +++
>  .../test/315_capsule_missing_payload.dts      |  19 +++
>  13 files changed, 546 insertions(+)
>  create mode 100644 include/sandbox_efi_capsule.h
>  create mode 100644 tools/binman/etype/efi_capsule.py
>  create mode 100644 tools/binman/test/307_capsule.dts
>  create mode 100644 tools/binman/test/308_capsule_signed.dts
>  create mode 100644 tools/binman/test/309_capsule_version.dts
>  create mode 100644 tools/binman/test/310_capsule_signed_ver.dts
>  create mode 100644 tools/binman/test/311_capsule_oemflags.dts
>  create mode 100644 tools/binman/test/312_capsule_missing_key.dts
>  create mode 100644 tools/binman/test/313_capsule_missing_index.dts
>  create mode 100644 tools/binman/test/314_capsule_missing_guid.dts
>  create mode 100644 tools/binman/test/315_capsule_missing_payload.dts
>
> diff --git a/include/sandbox_efi_capsule.h b/include/sandbox_efi_capsule.h
> new file mode 100644
> index 0000000000..da71b87a18
> --- /dev/null
> +++ b/include/sandbox_efi_capsule.h
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2023, Linaro Limited
> + */
> +
> +#if !defined(_SANDBOX_EFI_CAPSULE_H_)
> +#define _SANDBOX_EFI_CAPSULE_H_
> +
> +#define SANDBOX_UBOOT_IMAGE_GUID       "09d7cf52-0720-4710-91d1-08469b7fe9c8"
> +#define SANDBOX_UBOOT_ENV_IMAGE_GUID   "5a7021f5-fef2-48b4-aaba-832e777418c0"
> +#define SANDBOX_FIT_IMAGE_GUID         "3673b45d-6a7c-46f3-9e60-adabb03f7937"
> +#define SANDBOX_INCORRECT_GUID         "058b7d83-50d5-4c47-a195-60d86ad341c4"

These should not be needed in binman tests.
> +
> +#endif /* _SANDBOX_EFI_CAPSULE_H_ */
> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
> index f2376932be..fc094b9c08 100644
> --- a/tools/binman/entries.rst
> +++ b/tools/binman/entries.rst
> @@ -468,6 +468,68 @@ updating the EC on startup via software sync.
>
>
>
> +.. _etype_efi_capsule:
> +
> +Entry: capsule: Entry for generating EFI Capsule files
> +------------------------------------------------------
> +
> +The parameters needed for generation of the capsules can
> +be provided as properties in the entry.
> +
> +Properties / Entry arguments:
> +    - image-index: Unique number for identifying corresponding
> +      payload image. Number between 1 and descriptor count, i.e.
> +      the total number of firmware images that can be updated.
> +    - image-type-id: Image GUID which will be used for identifying the
> +      updatable image on the board.
> +    - hardware-instance: Optional number for identifying unique
> +      hardware instance of a device in the system. Default value of 0
> +      for images where value is not to be used.
> +    - fw-version: Optional value of image version that can be put on
> +      the capsule through the Firmware Management Protocol(FMP) header.
> +    - monotonic-count: Count used when signing an image.
> +    - private-key: Path to PEM formatted .key private key file. This property
> +      is mandatory for generating signed capsules.
> +    - public-key-cert: Path to PEM formatted .crt public key certificate
> +      file. This property is mandatory for generating signed capsules.
> +    - oem-flags - Optional OEM flags to be passed through capsule header.
> +
> +    Since this is a subclass of Entry_section, all properties of the parent
> +    class also apply here.
> +
> +For more details on the description of the capsule format, and the capsule
> +update functionality, refer Section 8.5 and Chapter 23 in the `UEFI
> +specification`_.
> +
> +The capsule parameters like image index and image GUID are passed as
> +properties in the entry. The payload to be used in the capsule is to be
> +provided as a subnode of the capsule entry.
> +
> +A typical capsule entry node would then look something like this::
> +
> +    capsule {
> +            type = "efi-capsule";
> +            image-index = <0x1>;
> +            /* Image GUID for testing capsule update */
> +            image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
> +            hardware-instance = <0x0>;
> +            private-key = "path/to/the/private/key";
> +            public-key-cert = "path/to/the/public-key-cert";
> +            oem-flags = <0x8000>;
> +
> +            u-boot {
> +            };
> +    };
> +
> +In the above example, the capsule payload is the U-Boot image. The
> +capsule entry would read the contents of the payload and put them
> +into the capsule. Any external file can also be specified as the
> +payload using the blob-ext subnode.
> +
> +.. _`UEFI specification`: https://uefi.org/sites/default/files/resources/UEFI_Spec_2_10_Aug29.pdf
> +
> +
> +
>  .. _etype_encrypted:
>
>  Entry: encrypted: Externally built encrypted binary blob
> diff --git a/tools/binman/etype/efi_capsule.py b/tools/binman/etype/efi_capsule.py
> new file mode 100644
> index 0000000000..bfdca94e26
> --- /dev/null
> +++ b/tools/binman/etype/efi_capsule.py
> @@ -0,0 +1,143 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +# Copyright (c) 2023 Linaro Limited
> +#
> +# Entry-type module for producing a EFI capsule
> +#
> +
> +import os
> +
> +from binman.entry import Entry
> +from binman.etype.section import Entry_section
> +from dtoc import fdt_util
> +from u_boot_pylib import tools
> +
> +class Entry_efi_capsule(Entry_section):
> +    """Generate EFI capsules
> +
> +    The parameters needed for generation of the capsules can
> +    be provided as properties in the entry.
> +
> +    Properties / Entry arguments:
> +    - image-index: Unique number for identifying corresponding
> +      payload image. Number between 1 and descriptor count, i.e.
> +      the total number of firmware images that can be updated.
> +    - image-type-id: Image GUID which will be used for identifying the
> +      updatable image on the board.
> +    - hardware-instance: Optional number for identifying unique
> +      hardware instance of a device in the system. Default value of 0
> +      for images where value is not to be used.
> +    - fw-version: Optional value of image version that can be put on
> +      the capsule through the Firmware Management Protocol(FMP) header.
> +    - monotonic-count: Count used when signing an image.
> +    - private-key: Path to PEM formatted .key private key file. This property
> +      is mandatory for generating signed capsules.
> +    - public-key-cert: Path to PEM formatted .crt public key certificate
> +      file. This property is mandatory for generating signed capsules.
> +    - oem-flags - Optional OEM flags to be passed through capsule header.
> +
> +    Since this is a subclass of Entry_section, all properties of the parent
> +    class also apply here.
> +
> +    For more details on the description of the capsule format, and the capsule
> +    update functionality, refer Section 8.5 and Chapter 23 in the `UEFI
> +    specification`_.
> +
> +    The capsule parameters like image index and image GUID are passed as
> +    properties in the entry. The payload to be used in the capsule is to be
> +    provided as a subnode of the capsule entry.
> +
> +    A typical capsule entry node would then look something like this
> +
> +    capsule {
> +            type = "efi-capsule";
> +            image-index = <0x1>;
> +            /* Image GUID for testing capsule update */
> +            image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
> +            hardware-instance = <0x0>;
> +            private-key = "path/to/the/private/key";
> +            public-key-cert = "path/to/the/public-key-cert";
> +            oem-flags = <0x8000>;
> +
> +            u-boot {
> +            };
> +    };
> +
> +    In the above example, the capsule payload is the U-Boot image. The
> +    capsule entry would read the contents of the payload and put them
> +    into the capsule. Any external file can also be specified as the
> +    payload using the blob-ext subnode.
> +
> +    .. _`UEFI specification`: https://uefi.org/sites/default/files/resources/UEFI_Spec_2_10_Aug29.pdf
> +    """
> +    def __init__(self, section, etype, node):
> +        super().__init__(section, etype, node)
> +        self.required_props = ['image-index', 'image-type-id']
> +        self.image_index = 0
> +        self.image_guid = ''
> +        self.hardware_instance = 0
> +        self.monotonic_count = 0
> +        self.fw_version = 0
> +        self.oem_flags = 0
> +        self.private_key = ''
> +        self.public_key_cert = ''
> +        self.auth = 0
> +
> +    def ReadNode(self):
> +        self.ReadEntries()
> +        super().ReadNode()

I believe those two lines should be swapped.

> +
> +        self.image_index = fdt_util.GetInt(self._node, 'image-index')
> +        self.image_guid = fdt_util.GetString(self._node, 'image-type-id')
> +        self.fw_version = fdt_util.GetInt(self._node, 'fw-version')
> +        self.hardware_instance = fdt_util.GetInt(self._node, 'hardware-instance')
> +        self.monotonic_count = fdt_util.GetInt(self._node, 'monotonic-count')
> +        self.oem_flags = fdt_util.GetInt(self._node, 'oem-flags')
> +
> +        self.private_key = fdt_util.GetString(self._node, 'private-key')
> +        self.public_key_cert = fdt_util.GetString(self._node, 'public-key-cert')
> +        if ((self.private_key and not self.public_key_cert) or (self.public_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.public_key_cert):
> +            self.auth = 0
> +        else:
> +            self.auth = 1
> +
> +    def ReadEntries(self):
> +        """Read the subnode to get the payload for this capsule"""
> +        # We can have a single payload per capsule
> +        if len(self._node.subnodes) == 0:
> +            self.Raise('The capsule entry expects at least one subnode for payload')

Still need to drop this

> +
> +        for node in self._node.subnodes:
> +            entry = Entry.Create(self, node)
> +            entry.ReadNode()
> +            self._entries[entry.name] = entry

I think you can drop this method, since it should be the same as entry_Sectoin ?

> +
> +    def BuildSectionData(self, required):
> +        private_key = ''
> +        public_key_cert = ''
> +        if self.auth:
> +            if not os.path.isabs(self.private_key):
> +                private_key =  tools.get_input_filename(self.private_key)
> +            if not os.path.isabs(self.public_key_cert):
> +                public_key_cert = tools.get_input_filename(self.public_key_cert)
> +        data, payload, _ = self.collect_contents_to_file(

s/_/uniq/ since you need this below

> +            self._entries.values(), 'capsule_payload')

'capsule_in' is better as it is an input to this entry type

> +        outfile = self._filename if self._filename else self._node.name

You need a unique filename so cannot use self._node.name since it is
not guaranteed to be unique.

See how mkimage does this, using uniq

> +        capsule_fname = tools.get_output_filename(outfile)
> +        ret = self.mkeficapsule.generate_capsule(self.image_index,
> +                                                  self.image_guid,
> +                                                  self.hardware_instance,
> +                                                  payload,
> +                                                  capsule_fname,
> +                                                  private_key,
> +                                                  public_key_cert,
> +                                                  self.monotonic_count,
> +                                                  self.fw_version,
> +                                                  self.oem_flags)
> +        if ret is not None:
> +            os.remove(payload)
> +            return tools.read_file(capsule_fname)
> +
> +    def AddBintools(self, btools):
> +        self.mkeficapsule = self.AddBintool(btools, 'mkeficapsule')
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index 36428ec343..9bda0157f7 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -48,6 +48,7 @@ U_BOOT_VPL_DATA       = b'vpl76543210fedcbazywxyz_'
>  BLOB_DATA             = b'89'
>  ME_DATA               = b'0abcd'
>  VGA_DATA              = b'vga'
> +EFI_CAPSULE_DATA      = b'efi'
>  U_BOOT_DTB_DATA       = b'udtb'
>  U_BOOT_SPL_DTB_DATA   = b'spldtb'
>  U_BOOT_TPL_DTB_DATA   = b'tpldtb'
> @@ -119,6 +120,11 @@ COMP_BINTOOLS = ['bzip2', 'gzip', 'lz4', 'lzma_alone', 'lzop', 'xz', 'zstd']
>
>  TEE_ADDR = 0x5678
>
> +# Firmware Management Protocol(FMP) GUID
> +FW_MGMT_GUID = 'edd5cb6d2de8444cbda17194199ad92a'
> +# Image GUID specified in the DTS
> +CAPSULE_IMAGE_GUID = '52cfd7092007104791d108469b7fe9c8'
> +
>  class TestFunctional(unittest.TestCase):
>      """Functional tests for binman
>
> @@ -215,6 +221,7 @@ class TestFunctional(unittest.TestCase):
>          TestFunctional._MakeInputFile('scp.bin', SCP_DATA)
>          TestFunctional._MakeInputFile('rockchip-tpl.bin', ROCKCHIP_TPL_DATA)
>          TestFunctional._MakeInputFile('ti_unsecure.bin', TI_UNSECURE_DATA)
> +        TestFunctional._MakeInputFile('capsule_input.bin', EFI_CAPSULE_DATA)
>
>          # Add a few .dtb files for testing
>          TestFunctional._MakeInputFile('%s/test-fdt1.dtb' % TEST_FDT_SUBDIR,
> @@ -7139,5 +7146,119 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
>           self.assertEqual(fdt_util.GetString(key_node, "key-name-hint"),
>                            "key")
>
> +    def _CheckCapsule(self, data, signed_capsule=False, version_check=False,
> +                      capoemflags=False):
> +        fmp_signature = "4d535331" # 'M', 'S', 'S', '1'
> +        fmp_size = "10"
> +        fmp_fw_version = "02"
> +        oemflag = "0080"
> +
> +        payload_data = EFI_CAPSULE_DATA
> +
> +        # Firmware Management Protocol(FMP) GUID - offset(0 - 32)
> +        self.assertEqual(FW_MGMT_GUID, data.hex()[:32])
> +        # Image GUID - offset(96 - 128)
> +        self.assertEqual(CAPSULE_IMAGE_GUID, data.hex()[96:128])
> +

As discussed please add a TODO here, so it is clear that you are
planning to write a decoder tool like dump_image.

> +        if capoemflags:
> +            # OEM Flags - offset(40 - 44)
> +            self.assertEqual(oemflag, data.hex()[40:44])
> +        if signed_capsule and version_check:
> +            # FMP header signature - offset(4770 - 4778)
> +            self.assertEqual(fmp_signature, data.hex()[4770:4778])
> +            # FMP header size - offset(4778 - 4780)
> +            self.assertEqual(fmp_size, data.hex()[4778:4780])
> +            # firmware version - offset(4786 - 4788)
> +            self.assertEqual(fmp_fw_version, data.hex()[4786:4788])
> +            # payload offset signed capsule(4802 - 4808)
> +            self.assertEqual(payload_data.hex(), data.hex()[4802:4808])
> +        elif signed_capsule:
> +            # payload offset signed capsule(4770 - 4776)
> +            self.assertEqual(payload_data.hex(), data.hex()[4770:4776])
> +        elif version_check:
> +            # FMP header signature - offset(184 - 192)
> +            self.assertEqual(fmp_signature, data.hex()[184:192])
> +            # FMP header size - offset(192 - 194)
> +            self.assertEqual(fmp_size, data.hex()[192:194])
> +            # firmware version - offset(200 - 202)
> +            self.assertEqual(fmp_fw_version, data.hex()[200:202])
> +            # payload offset for non-signed capsule with version header(216 - 222)
> +            self.assertEqual(payload_data.hex(), data.hex()[216:222])
> +        else:
> +            # payload offset for non-signed capsule with no version header(184 - 190)
> +            self.assertEqual(payload_data.hex(), data.hex()[184:190])
> +
> +    def testCapsuleGen(self):
> +        """Test generation of EFI capsule"""
> +        data = self._DoReadFile('307_capsule.dts')
> +
> +        self._CheckCapsule(data)
> +
> +    def testSignedCapsuleGen(self):
> +        """Test generation of EFI capsule"""
> +        data = tools.read_file(self.TestFile("key.key"))
> +        self._MakeInputFile("key.key", data)
> +        data = tools.read_file(self.TestFile("key.pem"))
> +        self._MakeInputFile("key.crt", data)
> +
> +        data = self._DoReadFile('308_capsule_signed.dts')
> +
> +        self._CheckCapsule(data, signed_capsule=True)
> +
> +    def testCapsuleGenVersionSupport(self):
> +        """Test generation of EFI capsule with version support"""
> +        data = self._DoReadFile('309_capsule_version.dts')
> +
> +        self._CheckCapsule(data, version_check=True)
> +
> +    def testCapsuleGenSignedVer(self):
> +        """Test generation of signed EFI capsule with version information"""
> +        data = tools.read_file(self.TestFile("key.key"))
> +        self._MakeInputFile("key.key", data)
> +        data = tools.read_file(self.TestFile("key.pem"))
> +        self._MakeInputFile("key.crt", data)
> +
> +        data = self._DoReadFile('310_capsule_signed_ver.dts')
> +
> +        self._CheckCapsule(data, signed_capsule=True, version_check=True)
> +
> +    def testCapsuleGenCapOemFlags(self):
> +        """Test generation of EFI capsule with OEM Flags set"""
> +        data = self._DoReadFile('311_capsule_oemflags.dts')
> +
> +        self._CheckCapsule(data, capoemflags=True)
> +
> +    def testCapsuleGenKeyMissing(self):
> +        """Test that binman errors out on missing key"""
> +        with self.assertRaises(ValueError) as e:
> +            self._DoReadFile('312_capsule_missing_key.dts')
> +
> +        self.assertIn("Both private key and public key certificate need to be provided",
> +                      str(e.exception))
> +
> +    def testCapsuleGenIndexMissing(self):
> +        """Test that binman errors out on missing image index"""
> +        with self.assertRaises(ValueError) as e:
> +            self._DoReadFile('313_capsule_missing_index.dts')
> +
> +        self.assertIn("entry is missing properties: image-index",
> +                      str(e.exception))
> +
> +    def testCapsuleGenGuidMissing(self):
> +        """Test that binman errors out on missing image GUID"""
> +        with self.assertRaises(ValueError) as e:
> +            self._DoReadFile('314_capsule_missing_guid.dts')
> +
> +        self.assertIn("entry is missing properties: image-type-id",
> +                      str(e.exception))
> +
> +    def testCapsuleGenPayloadMissing(self):
> +        """Test that binman errors out on missing input(payload)image"""
> +        with self.assertRaises(ValueError) as e:
> +            self._DoReadFile('315_capsule_missing_payload.dts')
> +
> +        self.assertIn("The capsule entry expects at least one subnode for payload",
> +                      str(e.exception))
> +
>  if __name__ == "__main__":
>      unittest.main()
> diff --git a/tools/binman/test/307_capsule.dts b/tools/binman/test/307_capsule.dts
> new file mode 100644
> index 0000000000..86552ff83f
> --- /dev/null
> +++ b/tools/binman/test/307_capsule.dts
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/dts-v1/;
> +
> +#include <sandbox_efi_capsule.h>

We can't include sandbox files in binman! I Does it actually matter
what the GUID value is? Can they all be the same? For now I think it
is best to have

#define BINMAN_TEST_GUID "xxx"

repeated at the top of each .dts file. Hopefully at some point we can
figure out a sensible way to provide a decoder ring for this mess, so
we can use actually names in the binman definition instead of GUIDs.

> +
> +/ {
> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +
> +       binman {
> +               efi-capsule {
> +                       image-index = <0x1>;
> +                       /* Image GUID for testing capsule update */
> +                       image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
> +                       hardware-instance = <0x0>;
> +
> +                       blob {
> +                               filename = "capsule_input.bin";
> +                       };
> +               };
> +       };
> +};


Regards,
Simon
Simon Glass Aug. 5, 2023, 5:20 p.m. UTC | #2 
Hi Sughosh,

On Sat, 5 Aug 2023 at 09:03, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > Add support in binman for generating EFI capsules. The capsule
> > parameters can be specified through the capsule binman entry. Also add
> > test cases in binman for testing capsule generation.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> > Changes since V6:
> > * Add macros for the GUID strings in sandbox_efi_capsule.h
> > * Highlight that the private and public keys are mandatory for capsule
> >   signing.
> > * Add a URL link to the UEFI spec, as used in the rst files.
> > * Use local vars for private and public keys in BuildSectionData()
> > * Use local vars for input payload and capsule filenames in
> >   BuildSectionData().
> > * Drop the ProcessContents() and SetImagePos() as the superclass
> >   functions suffice.
> > * Use GUID macro names in the capsule test dts files.
> > * Rename efi_capsule_payload.bin to capsule_input.bin.
> >
> >
> >  include/sandbox_efi_capsule.h                 |  14 ++
>
> Please move this file to a later patch - see below.
>
> Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.
>
> >  tools/binman/entries.rst                      |  62 ++++++++
> >  tools/binman/etype/efi_capsule.py             | 143 ++++++++++++++++++
> >  tools/binman/ftest.py                         | 121 +++++++++++++++
> >  tools/binman/test/307_capsule.dts             |  23 +++
> >  tools/binman/test/308_capsule_signed.dts      |  25 +++
> >  tools/binman/test/309_capsule_version.dts     |  24 +++
> >  tools/binman/test/310_capsule_signed_ver.dts  |  26 ++++
> >  tools/binman/test/311_capsule_oemflags.dts    |  24 +++
> >  tools/binman/test/312_capsule_missing_key.dts |  24 +++
> >  .../binman/test/313_capsule_missing_index.dts |  22 +++
> >  .../binman/test/314_capsule_missing_guid.dts  |  19 +++
> >  .../test/315_capsule_missing_payload.dts      |  19 +++
> >  13 files changed, 546 insertions(+)
> >  create mode 100644 include/sandbox_efi_capsule.h
> >  create mode 100644 tools/binman/etype/efi_capsule.py
> >  create mode 100644 tools/binman/test/307_capsule.dts
> >  create mode 100644 tools/binman/test/308_capsule_signed.dts
> >  create mode 100644 tools/binman/test/309_capsule_version.dts
> >  create mode 100644 tools/binman/test/310_capsule_signed_ver.dts
> >  create mode 100644 tools/binman/test/311_capsule_oemflags.dts
> >  create mode 100644 tools/binman/test/312_capsule_missing_key.dts
> >  create mode 100644 tools/binman/test/313_capsule_missing_index.dts
> >  create mode 100644 tools/binman/test/314_capsule_missing_guid.dts
> >  create mode 100644 tools/binman/test/315_capsule_missing_payload.dts
> >

[..]

> > diff --git a/tools/binman/test/307_capsule.dts b/tools/binman/test/307_capsule.dts
> > new file mode 100644
> > index 0000000000..86552ff83f
> > --- /dev/null
> > +++ b/tools/binman/test/307_capsule.dts
> > @@ -0,0 +1,23 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +/dts-v1/;
> > +
> > +#include <sandbox_efi_capsule.h>
>
> We can't include sandbox files in binman! I Does it actually matter
> what the GUID value is? Can they all be the same? For now I think it
> is best to have
>
> #define BINMAN_TEST_GUID "xxx"
>
> repeated at the top of each .dts file. Hopefully at some point we can
> figure out a sensible way to provide a decoder ring for this mess, so
> we can use actually names in the binman definition instead of GUIDs.

Oh, having thought about this a bit more, there is a much better way.

In the entry type, allow the image_guid to be a string, like
'binman-test' or 'sandbox-test'. Then binman can have a conversion
function to figure out the GUID to pass to the tool, e.g. in
efi_capsule.py:

TYPE_TO_GUID = {
   'binman-test': '09123987987f98712',
   'sandbox-test':, '98123987123123987123987',
}

def get_guid(type_str):
    'Get the GUID to use for a particular purpose"""
    return TYPE_TO_GUID.get(type_str, type_str)

Then you can use these same strings in the sandbox tests, without
needing to include anything.

>
> > +
> > +/ {
> > +       #address-cells = <1>;
> > +       #size-cells = <1>;
> > +
> > +       binman {
> > +               efi-capsule {
> > +                       image-index = <0x1>;
> > +                       /* Image GUID for testing capsule update */
> > +                       image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
> > +                       hardware-instance = <0x0>;
> > +
> > +                       blob {
> > +                               filename = "capsule_input.bin";
> > +                       };
> > +               };
> > +       };
> > +};

Regards,
Simon
Sughosh Ganu Aug. 5, 2023, 6:42 p.m. UTC | #3 
hi Simon,

On Sat, 5 Aug 2023 at 20:34, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > Add support in binman for generating EFI capsules. The capsule
> > parameters can be specified through the capsule binman entry. Also add
> > test cases in binman for testing capsule generation.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> > Changes since V6:
> > * Add macros for the GUID strings in sandbox_efi_capsule.h
> > * Highlight that the private and public keys are mandatory for capsule
> >   signing.
> > * Add a URL link to the UEFI spec, as used in the rst files.
> > * Use local vars for private and public keys in BuildSectionData()
> > * Use local vars for input payload and capsule filenames in
> >   BuildSectionData().
> > * Drop the ProcessContents() and SetImagePos() as the superclass
> >   functions suffice.
> > * Use GUID macro names in the capsule test dts files.
> > * Rename efi_capsule_payload.bin to capsule_input.bin.
> >
> >
> >  include/sandbox_efi_capsule.h                 |  14 ++
>
> Please move this file to a later patch - see below.

The idea was to also be able to run the binman capsule tests once this
patch was applied. If we are to move this to a separate patch, it
should be the one before this patch. But I guess based on your other
reply, this might not be needed after all.

>
> Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.

Umm, I am not too sure. Maybe we can take a call at a later point if
there are too many files that start cropping up.

>
> >  tools/binman/entries.rst                      |  62 ++++++++
> >  tools/binman/etype/efi_capsule.py             | 143 ++++++++++++++++++
> >  tools/binman/ftest.py                         | 121 +++++++++++++++
> >  tools/binman/test/307_capsule.dts             |  23 +++
> >  tools/binman/test/308_capsule_signed.dts      |  25 +++
> >  tools/binman/test/309_capsule_version.dts     |  24 +++
> >  tools/binman/test/310_capsule_signed_ver.dts  |  26 ++++
> >  tools/binman/test/311_capsule_oemflags.dts    |  24 +++
> >  tools/binman/test/312_capsule_missing_key.dts |  24 +++
> >  .../binman/test/313_capsule_missing_index.dts |  22 +++
> >  .../binman/test/314_capsule_missing_guid.dts  |  19 +++
> >  .../test/315_capsule_missing_payload.dts      |  19 +++
> >  13 files changed, 546 insertions(+)
> >  create mode 100644 include/sandbox_efi_capsule.h
> >  create mode 100644 tools/binman/etype/efi_capsule.py
> >  create mode 100644 tools/binman/test/307_capsule.dts
> >  create mode 100644 tools/binman/test/308_capsule_signed.dts
> >  create mode 100644 tools/binman/test/309_capsule_version.dts
> >  create mode 100644 tools/binman/test/310_capsule_signed_ver.dts
> >  create mode 100644 tools/binman/test/311_capsule_oemflags.dts
> >  create mode 100644 tools/binman/test/312_capsule_missing_key.dts
> >  create mode 100644 tools/binman/test/313_capsule_missing_index.dts
> >  create mode 100644 tools/binman/test/314_capsule_missing_guid.dts
> >  create mode 100644 tools/binman/test/315_capsule_missing_payload.dts
> >
> > diff --git a/include/sandbox_efi_capsule.h b/include/sandbox_efi_capsule.h
> > new file mode 100644
> > index 0000000000..da71b87a18
> > --- /dev/null
> > +++ b/include/sandbox_efi_capsule.h
> > @@ -0,0 +1,14 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2023, Linaro Limited
> > + */
> > +
> > +#if !defined(_SANDBOX_EFI_CAPSULE_H_)
> > +#define _SANDBOX_EFI_CAPSULE_H_
> > +
> > +#define SANDBOX_UBOOT_IMAGE_GUID       "09d7cf52-0720-4710-91d1-08469b7fe9c8"
> > +#define SANDBOX_UBOOT_ENV_IMAGE_GUID   "5a7021f5-fef2-48b4-aaba-832e777418c0"
> > +#define SANDBOX_FIT_IMAGE_GUID         "3673b45d-6a7c-46f3-9e60-adabb03f7937"
> > +#define SANDBOX_INCORRECT_GUID         "058b7d83-50d5-4c47-a195-60d86ad341c4"
>
> These should not be needed in binman tests.
> > +
> > +#endif /* _SANDBOX_EFI_CAPSULE_H_ */
> > diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
> > index f2376932be..fc094b9c08 100644
> > --- a/tools/binman/entries.rst
> > +++ b/tools/binman/entries.rst
> > @@ -468,6 +468,68 @@ updating the EC on startup via software sync.
> >
> >
> >
> > +.. _etype_efi_capsule:
> > +
> > +Entry: capsule: Entry for generating EFI Capsule files
> > +------------------------------------------------------
> > +
> > +The parameters needed for generation of the capsules can
> > +be provided as properties in the entry.
> > +
> > +Properties / Entry arguments:
> > +    - image-index: Unique number for identifying corresponding
> > +      payload image. Number between 1 and descriptor count, i.e.
> > +      the total number of firmware images that can be updated.
> > +    - image-type-id: Image GUID which will be used for identifying the
> > +      updatable image on the board.
> > +    - hardware-instance: Optional number for identifying unique
> > +      hardware instance of a device in the system. Default value of 0
> > +      for images where value is not to be used.
> > +    - fw-version: Optional value of image version that can be put on
> > +      the capsule through the Firmware Management Protocol(FMP) header.
> > +    - monotonic-count: Count used when signing an image.
> > +    - private-key: Path to PEM formatted .key private key file. This property
> > +      is mandatory for generating signed capsules.
> > +    - public-key-cert: Path to PEM formatted .crt public key certificate
> > +      file. This property is mandatory for generating signed capsules.
> > +    - oem-flags - Optional OEM flags to be passed through capsule header.
> > +
> > +    Since this is a subclass of Entry_section, all properties of the parent
> > +    class also apply here.
> > +
> > +For more details on the description of the capsule format, and the capsule
> > +update functionality, refer Section 8.5 and Chapter 23 in the `UEFI
> > +specification`_.
> > +
> > +The capsule parameters like image index and image GUID are passed as
> > +properties in the entry. The payload to be used in the capsule is to be
> > +provided as a subnode of the capsule entry.
> > +
> > +A typical capsule entry node would then look something like this::
> > +
> > +    capsule {
> > +            type = "efi-capsule";
> > +            image-index = <0x1>;
> > +            /* Image GUID for testing capsule update */
> > +            image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
> > +            hardware-instance = <0x0>;
> > +            private-key = "path/to/the/private/key";
> > +            public-key-cert = "path/to/the/public-key-cert";
> > +            oem-flags = <0x8000>;
> > +
> > +            u-boot {
> > +            };
> > +    };
> > +
> > +In the above example, the capsule payload is the U-Boot image. The
> > +capsule entry would read the contents of the payload and put them
> > +into the capsule. Any external file can also be specified as the
> > +payload using the blob-ext subnode.
> > +
> > +.. _`UEFI specification`: https://uefi.org/sites/default/files/resources/UEFI_Spec_2_10_Aug29.pdf
> > +
> > +
> > +
> >  .. _etype_encrypted:
> >
> >  Entry: encrypted: Externally built encrypted binary blob
> > diff --git a/tools/binman/etype/efi_capsule.py b/tools/binman/etype/efi_capsule.py
> > new file mode 100644
> > index 0000000000..bfdca94e26
> > --- /dev/null
> > +++ b/tools/binman/etype/efi_capsule.py
> > @@ -0,0 +1,143 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> > +# Copyright (c) 2023 Linaro Limited
> > +#
> > +# Entry-type module for producing a EFI capsule
> > +#
> > +
> > +import os
> > +
> > +from binman.entry import Entry
> > +from binman.etype.section import Entry_section
> > +from dtoc import fdt_util
> > +from u_boot_pylib import tools
> > +
> > +class Entry_efi_capsule(Entry_section):
> > +    """Generate EFI capsules
> > +
> > +    The parameters needed for generation of the capsules can
> > +    be provided as properties in the entry.
> > +
> > +    Properties / Entry arguments:
> > +    - image-index: Unique number for identifying corresponding
> > +      payload image. Number between 1 and descriptor count, i.e.
> > +      the total number of firmware images that can be updated.
> > +    - image-type-id: Image GUID which will be used for identifying the
> > +      updatable image on the board.
> > +    - hardware-instance: Optional number for identifying unique
> > +      hardware instance of a device in the system. Default value of 0
> > +      for images where value is not to be used.
> > +    - fw-version: Optional value of image version that can be put on
> > +      the capsule through the Firmware Management Protocol(FMP) header.
> > +    - monotonic-count: Count used when signing an image.
> > +    - private-key: Path to PEM formatted .key private key file. This property
> > +      is mandatory for generating signed capsules.
> > +    - public-key-cert: Path to PEM formatted .crt public key certificate
> > +      file. This property is mandatory for generating signed capsules.
> > +    - oem-flags - Optional OEM flags to be passed through capsule header.
> > +
> > +    Since this is a subclass of Entry_section, all properties of the parent
> > +    class also apply here.
> > +
> > +    For more details on the description of the capsule format, and the capsule
> > +    update functionality, refer Section 8.5 and Chapter 23 in the `UEFI
> > +    specification`_.
> > +
> > +    The capsule parameters like image index and image GUID are passed as
> > +    properties in the entry. The payload to be used in the capsule is to be
> > +    provided as a subnode of the capsule entry.
> > +
> > +    A typical capsule entry node would then look something like this
> > +
> > +    capsule {
> > +            type = "efi-capsule";
> > +            image-index = <0x1>;
> > +            /* Image GUID for testing capsule update */
> > +            image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
> > +            hardware-instance = <0x0>;
> > +            private-key = "path/to/the/private/key";
> > +            public-key-cert = "path/to/the/public-key-cert";
> > +            oem-flags = <0x8000>;
> > +
> > +            u-boot {
> > +            };
> > +    };
> > +
> > +    In the above example, the capsule payload is the U-Boot image. The
> > +    capsule entry would read the contents of the payload and put them
> > +    into the capsule. Any external file can also be specified as the
> > +    payload using the blob-ext subnode.
> > +
> > +    .. _`UEFI specification`: https://uefi.org/sites/default/files/resources/UEFI_Spec_2_10_Aug29.pdf
> > +    """
> > +    def __init__(self, section, etype, node):
> > +        super().__init__(section, etype, node)
> > +        self.required_props = ['image-index', 'image-type-id']
> > +        self.image_index = 0
> > +        self.image_guid = ''
> > +        self.hardware_instance = 0
> > +        self.monotonic_count = 0
> > +        self.fw_version = 0
> > +        self.oem_flags = 0
> > +        self.private_key = ''
> > +        self.public_key_cert = ''
> > +        self.auth = 0
> > +
> > +    def ReadNode(self):
> > +        self.ReadEntries()
> > +        super().ReadNode()
>
> I believe those two lines should be swapped.

Again, like my earlier code for ProcessContents() and SetImagePos(),
which was taken from mkimage.py as reference, this code is on similar
lines to what is in intel_ifwi.py. Both these files are authored by
you, so I took this as reference, especially mkimage.py.

>
> > +
> > +        self.image_index = fdt_util.GetInt(self._node, 'image-index')
> > +        self.image_guid = fdt_util.GetString(self._node, 'image-type-id')
> > +        self.fw_version = fdt_util.GetInt(self._node, 'fw-version')
> > +        self.hardware_instance = fdt_util.GetInt(self._node, 'hardware-instance')
> > +        self.monotonic_count = fdt_util.GetInt(self._node, 'monotonic-count')
> > +        self.oem_flags = fdt_util.GetInt(self._node, 'oem-flags')
> > +
> > +        self.private_key = fdt_util.GetString(self._node, 'private-key')
> > +        self.public_key_cert = fdt_util.GetString(self._node, 'public-key-cert')
> > +        if ((self.private_key and not self.public_key_cert) or (self.public_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.public_key_cert):
> > +            self.auth = 0
> > +        else:
> > +            self.auth = 1
> > +
> > +    def ReadEntries(self):
> > +        """Read the subnode to get the payload for this capsule"""
> > +        # We can have a single payload per capsule
> > +        if len(self._node.subnodes) == 0:
> > +            self.Raise('The capsule entry expects at least one subnode for payload')
>
> Still need to drop this

?
Should we not check if the input payload is missing? We cannot call
the mkeficapsule tool without an input image(binary).

>
> > +
> > +        for node in self._node.subnodes:
> > +            entry = Entry.Create(self, node)
> > +            entry.ReadNode()
> > +            self._entries[entry.name] = entry
>
> I think you can drop this method, since it should be the same as entry_Sectoin ?

Will check, but again, referenced from mkimage.py.

>
> > +
> > +    def BuildSectionData(self, required):
> > +        private_key = ''
> > +        public_key_cert = ''
> > +        if self.auth:
> > +            if not os.path.isabs(self.private_key):
> > +                private_key =  tools.get_input_filename(self.private_key)
> > +            if not os.path.isabs(self.public_key_cert):
> > +                public_key_cert = tools.get_input_filename(self.public_key_cert)
> > +        data, payload, _ = self.collect_contents_to_file(
>
> s/_/uniq/ since you need this below
>
> > +            self._entries.values(), 'capsule_payload')
>
> 'capsule_in' is better as it is an input to this entry type
>
> > +        outfile = self._filename if self._filename else self._node.name
>
> You need a unique filename so cannot use self._node.name since it is
> not guaranteed to be unique.
>
> See how mkimage does this, using uniq

Okay

>
> > +        capsule_fname = tools.get_output_filename(outfile)
> > +        ret = self.mkeficapsule.generate_capsule(self.image_index,
> > +                                                  self.image_guid,
> > +                                                  self.hardware_instance,
> > +                                                  payload,
> > +                                                  capsule_fname,
> > +                                                  private_key,
> > +                                                  public_key_cert,
> > +                                                  self.monotonic_count,
> > +                                                  self.fw_version,
> > +                                                  self.oem_flags)
> > +        if ret is not None:
> > +            os.remove(payload)
> > +            return tools.read_file(capsule_fname)
> > +
> > +    def AddBintools(self, btools):
> > +        self.mkeficapsule = self.AddBintool(btools, 'mkeficapsule')
> > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> > index 36428ec343..9bda0157f7 100644
> > --- a/tools/binman/ftest.py
> > +++ b/tools/binman/ftest.py
> > @@ -48,6 +48,7 @@ U_BOOT_VPL_DATA       = b'vpl76543210fedcbazywxyz_'
> >  BLOB_DATA             = b'89'
> >  ME_DATA               = b'0abcd'
> >  VGA_DATA              = b'vga'
> > +EFI_CAPSULE_DATA      = b'efi'
> >  U_BOOT_DTB_DATA       = b'udtb'
> >  U_BOOT_SPL_DTB_DATA   = b'spldtb'
> >  U_BOOT_TPL_DTB_DATA   = b'tpldtb'
> > @@ -119,6 +120,11 @@ COMP_BINTOOLS = ['bzip2', 'gzip', 'lz4', 'lzma_alone', 'lzop', 'xz', 'zstd']
> >
> >  TEE_ADDR = 0x5678
> >
> > +# Firmware Management Protocol(FMP) GUID
> > +FW_MGMT_GUID = 'edd5cb6d2de8444cbda17194199ad92a'
> > +# Image GUID specified in the DTS
> > +CAPSULE_IMAGE_GUID = '52cfd7092007104791d108469b7fe9c8'
> > +
> >  class TestFunctional(unittest.TestCase):
> >      """Functional tests for binman
> >
> > @@ -215,6 +221,7 @@ class TestFunctional(unittest.TestCase):
> >          TestFunctional._MakeInputFile('scp.bin', SCP_DATA)
> >          TestFunctional._MakeInputFile('rockchip-tpl.bin', ROCKCHIP_TPL_DATA)
> >          TestFunctional._MakeInputFile('ti_unsecure.bin', TI_UNSECURE_DATA)
> > +        TestFunctional._MakeInputFile('capsule_input.bin', EFI_CAPSULE_DATA)
> >
> >          # Add a few .dtb files for testing
> >          TestFunctional._MakeInputFile('%s/test-fdt1.dtb' % TEST_FDT_SUBDIR,
> > @@ -7139,5 +7146,119 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
> >           self.assertEqual(fdt_util.GetString(key_node, "key-name-hint"),
> >                            "key")
> >
> > +    def _CheckCapsule(self, data, signed_capsule=False, version_check=False,
> > +                      capoemflags=False):
> > +        fmp_signature = "4d535331" # 'M', 'S', 'S', '1'
> > +        fmp_size = "10"
> > +        fmp_fw_version = "02"
> > +        oemflag = "0080"
> > +
> > +        payload_data = EFI_CAPSULE_DATA
> > +
> > +        # Firmware Management Protocol(FMP) GUID - offset(0 - 32)
> > +        self.assertEqual(FW_MGMT_GUID, data.hex()[:32])
> > +        # Image GUID - offset(96 - 128)
> > +        self.assertEqual(CAPSULE_IMAGE_GUID, data.hex()[96:128])
> > +
>
> As discussed please add a TODO here, so it is clear that you are
> planning to write a decoder tool like dump_image.

Okay

-sughosh

>
> > +        if capoemflags:
> > +            # OEM Flags - offset(40 - 44)
> > +            self.assertEqual(oemflag, data.hex()[40:44])
> > +        if signed_capsule and version_check:
> > +            # FMP header signature - offset(4770 - 4778)
> > +            self.assertEqual(fmp_signature, data.hex()[4770:4778])
> > +            # FMP header size - offset(4778 - 4780)
> > +            self.assertEqual(fmp_size, data.hex()[4778:4780])
> > +            # firmware version - offset(4786 - 4788)
> > +            self.assertEqual(fmp_fw_version, data.hex()[4786:4788])
> > +            # payload offset signed capsule(4802 - 4808)
> > +            self.assertEqual(payload_data.hex(), data.hex()[4802:4808])
> > +        elif signed_capsule:
> > +            # payload offset signed capsule(4770 - 4776)
> > +            self.assertEqual(payload_data.hex(), data.hex()[4770:4776])
> > +        elif version_check:
> > +            # FMP header signature - offset(184 - 192)
> > +            self.assertEqual(fmp_signature, data.hex()[184:192])
> > +            # FMP header size - offset(192 - 194)
> > +            self.assertEqual(fmp_size, data.hex()[192:194])
> > +            # firmware version - offset(200 - 202)
> > +            self.assertEqual(fmp_fw_version, data.hex()[200:202])
> > +            # payload offset for non-signed capsule with version header(216 - 222)
> > +            self.assertEqual(payload_data.hex(), data.hex()[216:222])
> > +        else:
> > +            # payload offset for non-signed capsule with no version header(184 - 190)
> > +            self.assertEqual(payload_data.hex(), data.hex()[184:190])
> > +
> > +    def testCapsuleGen(self):
> > +        """Test generation of EFI capsule"""
> > +        data = self._DoReadFile('307_capsule.dts')
> > +
> > +        self._CheckCapsule(data)
> > +
> > +    def testSignedCapsuleGen(self):
> > +        """Test generation of EFI capsule"""
> > +        data = tools.read_file(self.TestFile("key.key"))
> > +        self._MakeInputFile("key.key", data)
> > +        data = tools.read_file(self.TestFile("key.pem"))
> > +        self._MakeInputFile("key.crt", data)
> > +
> > +        data = self._DoReadFile('308_capsule_signed.dts')
> > +
> > +        self._CheckCapsule(data, signed_capsule=True)
> > +
> > +    def testCapsuleGenVersionSupport(self):
> > +        """Test generation of EFI capsule with version support"""
> > +        data = self._DoReadFile('309_capsule_version.dts')
> > +
> > +        self._CheckCapsule(data, version_check=True)
> > +
> > +    def testCapsuleGenSignedVer(self):
> > +        """Test generation of signed EFI capsule with version information"""
> > +        data = tools.read_file(self.TestFile("key.key"))
> > +        self._MakeInputFile("key.key", data)
> > +        data = tools.read_file(self.TestFile("key.pem"))
> > +        self._MakeInputFile("key.crt", data)
> > +
> > +        data = self._DoReadFile('310_capsule_signed_ver.dts')
> > +
> > +        self._CheckCapsule(data, signed_capsule=True, version_check=True)
> > +
> > +    def testCapsuleGenCapOemFlags(self):
> > +        """Test generation of EFI capsule with OEM Flags set"""
> > +        data = self._DoReadFile('311_capsule_oemflags.dts')
> > +
> > +        self._CheckCapsule(data, capoemflags=True)
> > +
> > +    def testCapsuleGenKeyMissing(self):
> > +        """Test that binman errors out on missing key"""
> > +        with self.assertRaises(ValueError) as e:
> > +            self._DoReadFile('312_capsule_missing_key.dts')
> > +
> > +        self.assertIn("Both private key and public key certificate need to be provided",
> > +                      str(e.exception))
> > +
> > +    def testCapsuleGenIndexMissing(self):
> > +        """Test that binman errors out on missing image index"""
> > +        with self.assertRaises(ValueError) as e:
> > +            self._DoReadFile('313_capsule_missing_index.dts')
> > +
> > +        self.assertIn("entry is missing properties: image-index",
> > +                      str(e.exception))
> > +
> > +    def testCapsuleGenGuidMissing(self):
> > +        """Test that binman errors out on missing image GUID"""
> > +        with self.assertRaises(ValueError) as e:
> > +            self._DoReadFile('314_capsule_missing_guid.dts')
> > +
> > +        self.assertIn("entry is missing properties: image-type-id",
> > +                      str(e.exception))
> > +
> > +    def testCapsuleGenPayloadMissing(self):
> > +        """Test that binman errors out on missing input(payload)image"""
> > +        with self.assertRaises(ValueError) as e:
> > +            self._DoReadFile('315_capsule_missing_payload.dts')
> > +
> > +        self.assertIn("The capsule entry expects at least one subnode for payload",
> > +                      str(e.exception))
> > +
> >  if __name__ == "__main__":
> >      unittest.main()
> > diff --git a/tools/binman/test/307_capsule.dts b/tools/binman/test/307_capsule.dts
> > new file mode 100644
> > index 0000000000..86552ff83f
> > --- /dev/null
> > +++ b/tools/binman/test/307_capsule.dts
> > @@ -0,0 +1,23 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +/dts-v1/;
> > +
> > +#include <sandbox_efi_capsule.h>
>
> We can't include sandbox files in binman! I Does it actually matter
> what the GUID value is? Can they all be the same? For now I think it
> is best to have
>
> #define BINMAN_TEST_GUID "xxx"
>
> repeated at the top of each .dts file. Hopefully at some point we can
> figure out a sensible way to provide a decoder ring for this mess, so
> we can use actually names in the binman definition instead of GUIDs.
>
> > +
> > +/ {
> > +       #address-cells = <1>;
> > +       #size-cells = <1>;
> > +
> > +       binman {
> > +               efi-capsule {
> > +                       image-index = <0x1>;
> > +                       /* Image GUID for testing capsule update */
> > +                       image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
> > +                       hardware-instance = <0x0>;
> > +
> > +                       blob {
> > +                               filename = "capsule_input.bin";
> > +                       };
> > +               };
> > +       };
> > +};
>
>
> Regards,
> Simon
Sughosh Ganu Aug. 5, 2023, 6:43 p.m. UTC | #4 
hi Simon,

On Sat, 5 Aug 2023 at 22:50, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Sat, 5 Aug 2023 at 09:03, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > Add support in binman for generating EFI capsules. The capsule
> > > parameters can be specified through the capsule binman entry. Also add
> > > test cases in binman for testing capsule generation.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > > Changes since V6:
> > > * Add macros for the GUID strings in sandbox_efi_capsule.h
> > > * Highlight that the private and public keys are mandatory for capsule
> > >   signing.
> > > * Add a URL link to the UEFI spec, as used in the rst files.
> > > * Use local vars for private and public keys in BuildSectionData()
> > > * Use local vars for input payload and capsule filenames in
> > >   BuildSectionData().
> > > * Drop the ProcessContents() and SetImagePos() as the superclass
> > >   functions suffice.
> > > * Use GUID macro names in the capsule test dts files.
> > > * Rename efi_capsule_payload.bin to capsule_input.bin.
> > >
> > >
> > >  include/sandbox_efi_capsule.h                 |  14 ++
> >
> > Please move this file to a later patch - see below.
> >
> > Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.
> >
> > >  tools/binman/entries.rst                      |  62 ++++++++
> > >  tools/binman/etype/efi_capsule.py             | 143 ++++++++++++++++++
> > >  tools/binman/ftest.py                         | 121 +++++++++++++++
> > >  tools/binman/test/307_capsule.dts             |  23 +++
> > >  tools/binman/test/308_capsule_signed.dts      |  25 +++
> > >  tools/binman/test/309_capsule_version.dts     |  24 +++
> > >  tools/binman/test/310_capsule_signed_ver.dts  |  26 ++++
> > >  tools/binman/test/311_capsule_oemflags.dts    |  24 +++
> > >  tools/binman/test/312_capsule_missing_key.dts |  24 +++
> > >  .../binman/test/313_capsule_missing_index.dts |  22 +++
> > >  .../binman/test/314_capsule_missing_guid.dts  |  19 +++
> > >  .../test/315_capsule_missing_payload.dts      |  19 +++
> > >  13 files changed, 546 insertions(+)
> > >  create mode 100644 include/sandbox_efi_capsule.h
> > >  create mode 100644 tools/binman/etype/efi_capsule.py
> > >  create mode 100644 tools/binman/test/307_capsule.dts
> > >  create mode 100644 tools/binman/test/308_capsule_signed.dts
> > >  create mode 100644 tools/binman/test/309_capsule_version.dts
> > >  create mode 100644 tools/binman/test/310_capsule_signed_ver.dts
> > >  create mode 100644 tools/binman/test/311_capsule_oemflags.dts
> > >  create mode 100644 tools/binman/test/312_capsule_missing_key.dts
> > >  create mode 100644 tools/binman/test/313_capsule_missing_index.dts
> > >  create mode 100644 tools/binman/test/314_capsule_missing_guid.dts
> > >  create mode 100644 tools/binman/test/315_capsule_missing_payload.dts
> > >
>
> [..]
>
> > > diff --git a/tools/binman/test/307_capsule.dts b/tools/binman/test/307_capsule.dts
> > > new file mode 100644
> > > index 0000000000..86552ff83f
> > > --- /dev/null
> > > +++ b/tools/binman/test/307_capsule.dts
> > > @@ -0,0 +1,23 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +
> > > +/dts-v1/;
> > > +
> > > +#include <sandbox_efi_capsule.h>
> >
> > We can't include sandbox files in binman! I Does it actually matter
> > what the GUID value is? Can they all be the same? For now I think it
> > is best to have
> >
> > #define BINMAN_TEST_GUID "xxx"
> >
> > repeated at the top of each .dts file. Hopefully at some point we can
> > figure out a sensible way to provide a decoder ring for this mess, so
> > we can use actually names in the binman definition instead of GUIDs.
>
> Oh, having thought about this a bit more, there is a much better way.
>
> In the entry type, allow the image_guid to be a string, like
> 'binman-test' or 'sandbox-test'. Then binman can have a conversion
> function to figure out the GUID to pass to the tool, e.g. in
> efi_capsule.py:
>
> TYPE_TO_GUID = {
>    'binman-test': '09123987987f98712',
>    'sandbox-test':, '98123987123123987123987',
> }
>
> def get_guid(type_str):
>     'Get the GUID to use for a particular purpose"""
>     return TYPE_TO_GUID.get(type_str, type_str)
>
> Then you can use these same strings in the sandbox tests, without
> needing to include anything.

Okay, will try this out. Thanks for the suggestion.

-sughosh

>
> >
> > > +
> > > +/ {
> > > +       #address-cells = <1>;
> > > +       #size-cells = <1>;
> > > +
> > > +       binman {
> > > +               efi-capsule {
> > > +                       image-index = <0x1>;
> > > +                       /* Image GUID for testing capsule update */
> > > +                       image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
> > > +                       hardware-instance = <0x0>;
> > > +
> > > +                       blob {
> > > +                               filename = "capsule_input.bin";
> > > +                       };
> > > +               };
> > > +       };
> > > +};
>
> Regards,
> Simon
Simon Glass Aug. 5, 2023, 7:05 p.m. UTC | #5 
Hi Sughosh,

On Sat, 5 Aug 2023 at 12:42, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Sat, 5 Aug 2023 at 20:34, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > Add support in binman for generating EFI capsules. The capsule
> > > parameters can be specified through the capsule binman entry. Also add
> > > test cases in binman for testing capsule generation.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > > Changes since V6:
> > > * Add macros for the GUID strings in sandbox_efi_capsule.h
> > > * Highlight that the private and public keys are mandatory for capsule
> > >   signing.
> > > * Add a URL link to the UEFI spec, as used in the rst files.
> > > * Use local vars for private and public keys in BuildSectionData()
> > > * Use local vars for input payload and capsule filenames in
> > >   BuildSectionData().
> > > * Drop the ProcessContents() and SetImagePos() as the superclass
> > >   functions suffice.
> > > * Use GUID macro names in the capsule test dts files.
> > > * Rename efi_capsule_payload.bin to capsule_input.bin.
> > >
> > >
> > >  include/sandbox_efi_capsule.h                 |  14 ++
> >
> > Please move this file to a later patch - see below.
>
> The idea was to also be able to run the binman capsule tests once this
> patch was applied. If we are to move this to a separate patch, it
> should be the one before this patch. But I guess based on your other
> reply, this might not be needed after all.

Yes, it should not be needed if we name the test GUIDs. Remember that
binman is a standalone tool so cannot reference files outside
tools/...although there is no test for that so some things may have
crept in.

>
> >
> > Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.
>
> Umm, I am not too sure. Maybe we can take a call at a later point if
> there are too many files that start cropping up.

OK

>
> >
> > >  tools/binman/entries.rst                      |  62 ++++++++
> > >  tools/binman/etype/efi_capsule.py             | 143 ++++++++++++++++++
> > >  tools/binman/ftest.py                         | 121 +++++++++++++++
> > >  tools/binman/test/307_capsule.dts             |  23 +++
> > >  tools/binman/test/308_capsule_signed.dts      |  25 +++
> > >  tools/binman/test/309_capsule_version.dts     |  24 +++
> > >  tools/binman/test/310_capsule_signed_ver.dts  |  26 ++++
> > >  tools/binman/test/311_capsule_oemflags.dts    |  24 +++
> > >  tools/binman/test/312_capsule_missing_key.dts |  24 +++
> > >  .../binman/test/313_capsule_missing_index.dts |  22 +++
> > >  .../binman/test/314_capsule_missing_guid.dts  |  19 +++
> > >  .../test/315_capsule_missing_payload.dts      |  19 +++
> > >  13 files changed, 546 insertions(+)
> > >  create mode 100644 include/sandbox_efi_capsule.h
> > >  create mode 100644 tools/binman/etype/efi_capsule.py
> > >  create mode 100644 tools/binman/test/307_capsule.dts
> > >  create mode 100644 tools/binman/test/308_capsule_signed.dts
> > >  create mode 100644 tools/binman/test/309_capsule_version.dts
> > >  create mode 100644 tools/binman/test/310_capsule_signed_ver.dts
> > >  create mode 100644 tools/binman/test/311_capsule_oemflags.dts
> > >  create mode 100644 tools/binman/test/312_capsule_missing_key.dts
> > >  create mode 100644 tools/binman/test/313_capsule_missing_index.dts
> > >  create mode 100644 tools/binman/test/314_capsule_missing_guid.dts
> > >  create mode 100644 tools/binman/test/315_capsule_missing_payload.dts
> > >

[..]

> > > +
> > > +    def ReadNode(self):
> > > +        self.ReadEntries()
> > > +        super().ReadNode()
> >
> > I believe those two lines should be swapped.
>
> Again, like my earlier code for ProcessContents() and SetImagePos(),
> which was taken from mkimage.py as reference, this code is on similar
> lines to what is in intel_ifwi.py. Both these files are authored by
> you, so I took this as reference, especially mkimage.py.

OK, then take a look at mkimage.py and follow that. Yes intel_ifwi is
around the wrong way. Although these days ReadEntries() is called
automatically from entry_Section so you don't need to call it here.

>
> >
> > > +
> > > +        self.image_index = fdt_util.GetInt(self._node, 'image-index')
> > > +        self.image_guid = fdt_util.GetString(self._node, 'image-type-id')
> > > +        self.fw_version = fdt_util.GetInt(self._node, 'fw-version')
> > > +        self.hardware_instance = fdt_util.GetInt(self._node, 'hardware-instance')
> > > +        self.monotonic_count = fdt_util.GetInt(self._node, 'monotonic-count')
> > > +        self.oem_flags = fdt_util.GetInt(self._node, 'oem-flags')
> > > +
> > > +        self.private_key = fdt_util.GetString(self._node, 'private-key')
> > > +        self.public_key_cert = fdt_util.GetString(self._node, 'public-key-cert')
> > > +        if ((self.private_key and not self.public_key_cert) or (self.public_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.public_key_cert):
> > > +            self.auth = 0
> > > +        else:
> > > +            self.auth = 1
> > > +
> > > +    def ReadEntries(self):
> > > +        """Read the subnode to get the payload for this capsule"""
> > > +        # We can have a single payload per capsule
> > > +        if len(self._node.subnodes) == 0:
> > > +            self.Raise('The capsule entry expects at least one subnode for payload')
> >
> > Still need to drop this
>
> ?
> Should we not check if the input payload is missing? We cannot call
> the mkeficapsule tool without an input image(binary).

Why not?

>
> >
> > > +
> > > +        for node in self._node.subnodes:
> > > +            entry = Entry.Create(self, node)
> > > +            entry.ReadNode()
> > > +            self._entries[entry.name] = entry
> >
> > I think you can drop this method, since it should be the same as entry_Sectoin ?
>
> Will check, but again, referenced from mkimage.py.

That one is special since it has to deal with a special 'imagename' node.

Regards,
Simon
Sughosh Ganu Aug. 5, 2023, 7:35 p.m. UTC | #6 
hi Simon,

On Sun, 6 Aug 2023 at 00:35, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Sat, 5 Aug 2023 at 12:42, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Sat, 5 Aug 2023 at 20:34, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > Add support in binman for generating EFI capsules. The capsule
> > > > parameters can be specified through the capsule binman entry. Also add
> > > > test cases in binman for testing capsule generation.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > ---
> > > > Changes since V6:
> > > > * Add macros for the GUID strings in sandbox_efi_capsule.h
> > > > * Highlight that the private and public keys are mandatory for capsule
> > > >   signing.
> > > > * Add a URL link to the UEFI spec, as used in the rst files.
> > > > * Use local vars for private and public keys in BuildSectionData()
> > > > * Use local vars for input payload and capsule filenames in
> > > >   BuildSectionData().
> > > > * Drop the ProcessContents() and SetImagePos() as the superclass
> > > >   functions suffice.
> > > > * Use GUID macro names in the capsule test dts files.
> > > > * Rename efi_capsule_payload.bin to capsule_input.bin.
> > > >
> > > >
> > > >  include/sandbox_efi_capsule.h                 |  14 ++
> > >
> > > Please move this file to a later patch - see below.
> >
> > The idea was to also be able to run the binman capsule tests once this
> > patch was applied. If we are to move this to a separate patch, it
> > should be the one before this patch. But I guess based on your other
> > reply, this might not be needed after all.
>
> Yes, it should not be needed if we name the test GUIDs. Remember that
> binman is a standalone tool so cannot reference files outside
> tools/...although there is no test for that so some things may have
> crept in.
>
> >
> > >
> > > Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.
> >
> > Umm, I am not too sure. Maybe we can take a call at a later point if
> > there are too many files that start cropping up.
>
> OK
>
> >
> > >
> > > >  tools/binman/entries.rst                      |  62 ++++++++
> > > >  tools/binman/etype/efi_capsule.py             | 143 ++++++++++++++++++
> > > >  tools/binman/ftest.py                         | 121 +++++++++++++++
> > > >  tools/binman/test/307_capsule.dts             |  23 +++
> > > >  tools/binman/test/308_capsule_signed.dts      |  25 +++
> > > >  tools/binman/test/309_capsule_version.dts     |  24 +++
> > > >  tools/binman/test/310_capsule_signed_ver.dts  |  26 ++++
> > > >  tools/binman/test/311_capsule_oemflags.dts    |  24 +++
> > > >  tools/binman/test/312_capsule_missing_key.dts |  24 +++
> > > >  .../binman/test/313_capsule_missing_index.dts |  22 +++
> > > >  .../binman/test/314_capsule_missing_guid.dts  |  19 +++
> > > >  .../test/315_capsule_missing_payload.dts      |  19 +++
> > > >  13 files changed, 546 insertions(+)
> > > >  create mode 100644 include/sandbox_efi_capsule.h
> > > >  create mode 100644 tools/binman/etype/efi_capsule.py
> > > >  create mode 100644 tools/binman/test/307_capsule.dts
> > > >  create mode 100644 tools/binman/test/308_capsule_signed.dts
> > > >  create mode 100644 tools/binman/test/309_capsule_version.dts
> > > >  create mode 100644 tools/binman/test/310_capsule_signed_ver.dts
> > > >  create mode 100644 tools/binman/test/311_capsule_oemflags.dts
> > > >  create mode 100644 tools/binman/test/312_capsule_missing_key.dts
> > > >  create mode 100644 tools/binman/test/313_capsule_missing_index.dts
> > > >  create mode 100644 tools/binman/test/314_capsule_missing_guid.dts
> > > >  create mode 100644 tools/binman/test/315_capsule_missing_payload.dts
> > > >
>
> [..]
>
> > > > +
> > > > +    def ReadNode(self):
> > > > +        self.ReadEntries()
> > > > +        super().ReadNode()
> > >
> > > I believe those two lines should be swapped.
> >
> > Again, like my earlier code for ProcessContents() and SetImagePos(),
> > which was taken from mkimage.py as reference, this code is on similar
> > lines to what is in intel_ifwi.py. Both these files are authored by
> > you, so I took this as reference, especially mkimage.py.
>
> OK, then take a look at mkimage.py and follow that. Yes intel_ifwi is
> around the wrong way. Although these days ReadEntries() is called
> automatically from entry_Section so you don't need to call it here.
>
> >
> > >
> > > > +
> > > > +        self.image_index = fdt_util.GetInt(self._node, 'image-index')
> > > > +        self.image_guid = fdt_util.GetString(self._node, 'image-type-id')
> > > > +        self.fw_version = fdt_util.GetInt(self._node, 'fw-version')
> > > > +        self.hardware_instance = fdt_util.GetInt(self._node, 'hardware-instance')
> > > > +        self.monotonic_count = fdt_util.GetInt(self._node, 'monotonic-count')
> > > > +        self.oem_flags = fdt_util.GetInt(self._node, 'oem-flags')
> > > > +
> > > > +        self.private_key = fdt_util.GetString(self._node, 'private-key')
> > > > +        self.public_key_cert = fdt_util.GetString(self._node, 'public-key-cert')
> > > > +        if ((self.private_key and not self.public_key_cert) or (self.public_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.public_key_cert):
> > > > +            self.auth = 0
> > > > +        else:
> > > > +            self.auth = 1
> > > > +
> > > > +    def ReadEntries(self):
> > > > +        """Read the subnode to get the payload for this capsule"""
> > > > +        # We can have a single payload per capsule
> > > > +        if len(self._node.subnodes) == 0:
> > > > +            self.Raise('The capsule entry expects at least one subnode for payload')
> > >
> > > Still need to drop this
> >
> > ?
> > Should we not check if the input payload is missing? We cannot call
> > the mkeficapsule tool without an input image(binary).
>
> Why not?

The mkeficapsule tool expects a input binary(image blob as it calls
it) for generation of a normal capsule. Not passing the input image
and the capsule filename to the tool results in the help being
printed.
For e.g. the below command is not passing one filename.

$ ./tools/mkeficapsule -i 0x1 -g 09d7cf52-0720-4710-91d1-08469b7fe9c8
foo.capsule
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
-v, --fw-version <version>  firmware version
-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
-h, --help                  print a help message


So we need an input binary for a normal capsule.

-sughosh

>
> >
> > >
> > > > +
> > > > +        for node in self._node.subnodes:
> > > > +            entry = Entry.Create(self, node)
> > > > +            entry.ReadNode()
> > > > +            self._entries[entry.name] = entry
> > >
> > > I think you can drop this method, since it should be the same as entry_Sectoin ?
> >
> > Will check, but again, referenced from mkimage.py.
>
> That one is special since it has to deal with a special 'imagename' node.
>
> Regards,
> Simon
Simon Glass Aug. 5, 2023, 7:56 p.m. UTC | #7 
Hi Sughosh,

On Sat, 5 Aug 2023 at 13:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Sun, 6 Aug 2023 at 00:35, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Sat, 5 Aug 2023 at 12:42, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Sat, 5 Aug 2023 at 20:34, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > Add support in binman for generating EFI capsules. The capsule
> > > > > parameters can be specified through the capsule binman entry. Also add
> > > > > test cases in binman for testing capsule generation.
> > > > >
> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > ---
> > > > > Changes since V6:
> > > > > * Add macros for the GUID strings in sandbox_efi_capsule.h
> > > > > * Highlight that the private and public keys are mandatory for capsule
> > > > >   signing.
> > > > > * Add a URL link to the UEFI spec, as used in the rst files.
> > > > > * Use local vars for private and public keys in BuildSectionData()
> > > > > * Use local vars for input payload and capsule filenames in
> > > > >   BuildSectionData().
> > > > > * Drop the ProcessContents() and SetImagePos() as the superclass
> > > > >   functions suffice.
> > > > > * Use GUID macro names in the capsule test dts files.
> > > > > * Rename efi_capsule_payload.bin to capsule_input.bin.
> > > > >
> > > > >
> > > > >  include/sandbox_efi_capsule.h                 |  14 ++
> > > >
> > > > Please move this file to a later patch - see below.
> > >
> > > The idea was to also be able to run the binman capsule tests once this
> > > patch was applied. If we are to move this to a separate patch, it
> > > should be the one before this patch. But I guess based on your other
> > > reply, this might not be needed after all.
> >
> > Yes, it should not be needed if we name the test GUIDs. Remember that
> > binman is a standalone tool so cannot reference files outside
> > tools/...although there is no test for that so some things may have
> > crept in.
> >
> > >
> > > >
> > > > Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.
> > >
> > > Umm, I am not too sure. Maybe we can take a call at a later point if
> > > there are too many files that start cropping up.
> >
> > OK
> >
> > >
> > > >
> > > > >  tools/binman/entries.rst                      |  62 ++++++++
> > > > >  tools/binman/etype/efi_capsule.py             | 143 ++++++++++++++++++
> > > > >  tools/binman/ftest.py                         | 121 +++++++++++++++
> > > > >  tools/binman/test/307_capsule.dts             |  23 +++
> > > > >  tools/binman/test/308_capsule_signed.dts      |  25 +++
> > > > >  tools/binman/test/309_capsule_version.dts     |  24 +++
> > > > >  tools/binman/test/310_capsule_signed_ver.dts  |  26 ++++
> > > > >  tools/binman/test/311_capsule_oemflags.dts    |  24 +++
> > > > >  tools/binman/test/312_capsule_missing_key.dts |  24 +++
> > > > >  .../binman/test/313_capsule_missing_index.dts |  22 +++
> > > > >  .../binman/test/314_capsule_missing_guid.dts  |  19 +++
> > > > >  .../test/315_capsule_missing_payload.dts      |  19 +++
> > > > >  13 files changed, 546 insertions(+)
> > > > >  create mode 100644 include/sandbox_efi_capsule.h
> > > > >  create mode 100644 tools/binman/etype/efi_capsule.py
> > > > >  create mode 100644 tools/binman/test/307_capsule.dts
> > > > >  create mode 100644 tools/binman/test/308_capsule_signed.dts
> > > > >  create mode 100644 tools/binman/test/309_capsule_version.dts
> > > > >  create mode 100644 tools/binman/test/310_capsule_signed_ver.dts
> > > > >  create mode 100644 tools/binman/test/311_capsule_oemflags.dts
> > > > >  create mode 100644 tools/binman/test/312_capsule_missing_key.dts
> > > > >  create mode 100644 tools/binman/test/313_capsule_missing_index.dts
> > > > >  create mode 100644 tools/binman/test/314_capsule_missing_guid.dts
> > > > >  create mode 100644 tools/binman/test/315_capsule_missing_payload.dts
> > > > >
> >
> > [..]
> >
> > > > > +
> > > > > +    def ReadNode(self):
> > > > > +        self.ReadEntries()
> > > > > +        super().ReadNode()
> > > >
> > > > I believe those two lines should be swapped.
> > >
> > > Again, like my earlier code for ProcessContents() and SetImagePos(),
> > > which was taken from mkimage.py as reference, this code is on similar
> > > lines to what is in intel_ifwi.py. Both these files are authored by
> > > you, so I took this as reference, especially mkimage.py.
> >
> > OK, then take a look at mkimage.py and follow that. Yes intel_ifwi is
> > around the wrong way. Although these days ReadEntries() is called
> > automatically from entry_Section so you don't need to call it here.
> >
> > >
> > > >
> > > > > +
> > > > > +        self.image_index = fdt_util.GetInt(self._node, 'image-index')
> > > > > +        self.image_guid = fdt_util.GetString(self._node, 'image-type-id')
> > > > > +        self.fw_version = fdt_util.GetInt(self._node, 'fw-version')
> > > > > +        self.hardware_instance = fdt_util.GetInt(self._node, 'hardware-instance')
> > > > > +        self.monotonic_count = fdt_util.GetInt(self._node, 'monotonic-count')
> > > > > +        self.oem_flags = fdt_util.GetInt(self._node, 'oem-flags')
> > > > > +
> > > > > +        self.private_key = fdt_util.GetString(self._node, 'private-key')
> > > > > +        self.public_key_cert = fdt_util.GetString(self._node, 'public-key-cert')
> > > > > +        if ((self.private_key and not self.public_key_cert) or (self.public_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.public_key_cert):
> > > > > +            self.auth = 0
> > > > > +        else:
> > > > > +            self.auth = 1
> > > > > +
> > > > > +    def ReadEntries(self):
> > > > > +        """Read the subnode to get the payload for this capsule"""
> > > > > +        # We can have a single payload per capsule
> > > > > +        if len(self._node.subnodes) == 0:
> > > > > +            self.Raise('The capsule entry expects at least one subnode for payload')
> > > >
> > > > Still need to drop this
> > >
> > > ?
> > > Should we not check if the input payload is missing? We cannot call
> > > the mkeficapsule tool without an input image(binary).
> >
> > Why not?
>
> The mkeficapsule tool expects a input binary(image blob as it calls
> it) for generation of a normal capsule. Not passing the input image
> and the capsule filename to the tool results in the help being
> printed.
> For e.g. the below command is not passing one filename.
>
> $ ./tools/mkeficapsule -i 0x1 -g 09d7cf52-0720-4710-91d1-08469b7fe9c8
> foo.capsule
> 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
> -v, --fw-version <version>  firmware version
> -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
> -h, --help                  print a help message
>
>
> So we need an input binary for a normal capsule.

Yes of course I understand that, but it can be empty, I expect. If you
don't have any entries, then your etype implementation should use an
empty file, as written.

No other etype has this sort of restriction so I don't think we should
add it now.

Regards,
Simon
Sughosh Ganu Aug. 6, 2023, 3:35 p.m. UTC | #8 
hi Simon,

On Sat, 5 Aug 2023 at 22:50, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Sat, 5 Aug 2023 at 09:03, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > Add support in binman for generating EFI capsules. The capsule
> > > parameters can be specified through the capsule binman entry. Also add
> > > test cases in binman for testing capsule generation.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > > Changes since V6:
> > > * Add macros for the GUID strings in sandbox_efi_capsule.h
> > > * Highlight that the private and public keys are mandatory for capsule
> > >   signing.
> > > * Add a URL link to the UEFI spec, as used in the rst files.
> > > * Use local vars for private and public keys in BuildSectionData()
> > > * Use local vars for input payload and capsule filenames in
> > >   BuildSectionData().
> > > * Drop the ProcessContents() and SetImagePos() as the superclass
> > >   functions suffice.
> > > * Use GUID macro names in the capsule test dts files.
> > > * Rename efi_capsule_payload.bin to capsule_input.bin.
> > >
> > >
> > >  include/sandbox_efi_capsule.h                 |  14 ++
> >
> > Please move this file to a later patch - see below.
> >
> > Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.
> >
> > >  tools/binman/entries.rst                      |  62 ++++++++
> > >  tools/binman/etype/efi_capsule.py             | 143 ++++++++++++++++++
> > >  tools/binman/ftest.py                         | 121 +++++++++++++++
> > >  tools/binman/test/307_capsule.dts             |  23 +++
> > >  tools/binman/test/308_capsule_signed.dts      |  25 +++
> > >  tools/binman/test/309_capsule_version.dts     |  24 +++
> > >  tools/binman/test/310_capsule_signed_ver.dts  |  26 ++++
> > >  tools/binman/test/311_capsule_oemflags.dts    |  24 +++
> > >  tools/binman/test/312_capsule_missing_key.dts |  24 +++
> > >  .../binman/test/313_capsule_missing_index.dts |  22 +++
> > >  .../binman/test/314_capsule_missing_guid.dts  |  19 +++
> > >  .../test/315_capsule_missing_payload.dts      |  19 +++
> > >  13 files changed, 546 insertions(+)
> > >  create mode 100644 include/sandbox_efi_capsule.h
> > >  create mode 100644 tools/binman/etype/efi_capsule.py
> > >  create mode 100644 tools/binman/test/307_capsule.dts
> > >  create mode 100644 tools/binman/test/308_capsule_signed.dts
> > >  create mode 100644 tools/binman/test/309_capsule_version.dts
> > >  create mode 100644 tools/binman/test/310_capsule_signed_ver.dts
> > >  create mode 100644 tools/binman/test/311_capsule_oemflags.dts
> > >  create mode 100644 tools/binman/test/312_capsule_missing_key.dts
> > >  create mode 100644 tools/binman/test/313_capsule_missing_index.dts
> > >  create mode 100644 tools/binman/test/314_capsule_missing_guid.dts
> > >  create mode 100644 tools/binman/test/315_capsule_missing_payload.dts
> > >
>
> [..]
>
> > > diff --git a/tools/binman/test/307_capsule.dts b/tools/binman/test/307_capsule.dts
> > > new file mode 100644
> > > index 0000000000..86552ff83f
> > > --- /dev/null
> > > +++ b/tools/binman/test/307_capsule.dts
> > > @@ -0,0 +1,23 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +
> > > +/dts-v1/;
> > > +
> > > +#include <sandbox_efi_capsule.h>
> >
> > We can't include sandbox files in binman! I Does it actually matter
> > what the GUID value is? Can they all be the same? For now I think it
> > is best to have
> >
> > #define BINMAN_TEST_GUID "xxx"
> >
> > repeated at the top of each .dts file. Hopefully at some point we can
> > figure out a sensible way to provide a decoder ring for this mess, so
> > we can use actually names in the binman definition instead of GUIDs.
>
> Oh, having thought about this a bit more, there is a much better way.
>
> In the entry type, allow the image_guid to be a string, like
> 'binman-test' or 'sandbox-test'. Then binman can have a conversion
> function to figure out the GUID to pass to the tool, e.g. in
> efi_capsule.py:
>
> TYPE_TO_GUID = {
>    'binman-test': '09123987987f98712',
>    'sandbox-test':, '98123987123123987123987',
> }
>
> def get_guid(type_str):
>     'Get the GUID to use for a particular purpose"""
>     return TYPE_TO_GUID.get(type_str, type_str)
>
> Then you can use these same strings in the sandbox tests, without
> needing to include anything.

While we can use this method for both sandbox and binman tests, I
would prefer using the actual GUID strings for the sandbox tests. This
will serve as an illustrative example to the user for how to pass the
image GUID value for generating the capsule. For the binman tests, I
can use this logic to avoid including the header file.

-sughosh

>
> >
> > > +
> > > +/ {
> > > +       #address-cells = <1>;
> > > +       #size-cells = <1>;
> > > +
> > > +       binman {
> > > +               efi-capsule {
> > > +                       image-index = <0x1>;
> > > +                       /* Image GUID for testing capsule update */
> > > +                       image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
> > > +                       hardware-instance = <0x0>;
> > > +
> > > +                       blob {
> > > +                               filename = "capsule_input.bin";
> > > +                       };
> > > +               };
> > > +       };
> > > +};
>
> Regards,
> Simon
Simon Glass Aug. 6, 2023, 6:46 p.m. UTC | #9 
Hi Sughosh,

On Sun, 6 Aug 2023 at 09:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Sat, 5 Aug 2023 at 22:50, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Sat, 5 Aug 2023 at 09:03, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > Add support in binman for generating EFI capsules. The capsule
> > > > parameters can be specified through the capsule binman entry. Also add
> > > > test cases in binman for testing capsule generation.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > ---
> > > > Changes since V6:
> > > > * Add macros for the GUID strings in sandbox_efi_capsule.h
> > > > * Highlight that the private and public keys are mandatory for capsule
> > > >   signing.
> > > > * Add a URL link to the UEFI spec, as used in the rst files.
> > > > * Use local vars for private and public keys in BuildSectionData()
> > > > * Use local vars for input payload and capsule filenames in
> > > >   BuildSectionData().
> > > > * Drop the ProcessContents() and SetImagePos() as the superclass
> > > >   functions suffice.
> > > > * Use GUID macro names in the capsule test dts files.
> > > > * Rename efi_capsule_payload.bin to capsule_input.bin.
> > > >
> > > >
> > > >  include/sandbox_efi_capsule.h                 |  14 ++
> > >
> > > Please move this file to a later patch - see below.
> > >
> > > Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.
> > >
> > > >  tools/binman/entries.rst                      |  62 ++++++++
> > > >  tools/binman/etype/efi_capsule.py             | 143 ++++++++++++++++++
> > > >  tools/binman/ftest.py                         | 121 +++++++++++++++
> > > >  tools/binman/test/307_capsule.dts             |  23 +++
> > > >  tools/binman/test/308_capsule_signed.dts      |  25 +++
> > > >  tools/binman/test/309_capsule_version.dts     |  24 +++
> > > >  tools/binman/test/310_capsule_signed_ver.dts  |  26 ++++
> > > >  tools/binman/test/311_capsule_oemflags.dts    |  24 +++
> > > >  tools/binman/test/312_capsule_missing_key.dts |  24 +++
> > > >  .../binman/test/313_capsule_missing_index.dts |  22 +++
> > > >  .../binman/test/314_capsule_missing_guid.dts  |  19 +++
> > > >  .../test/315_capsule_missing_payload.dts      |  19 +++
> > > >  13 files changed, 546 insertions(+)
> > > >  create mode 100644 include/sandbox_efi_capsule.h
> > > >  create mode 100644 tools/binman/etype/efi_capsule.py
> > > >  create mode 100644 tools/binman/test/307_capsule.dts
> > > >  create mode 100644 tools/binman/test/308_capsule_signed.dts
> > > >  create mode 100644 tools/binman/test/309_capsule_version.dts
> > > >  create mode 100644 tools/binman/test/310_capsule_signed_ver.dts
> > > >  create mode 100644 tools/binman/test/311_capsule_oemflags.dts
> > > >  create mode 100644 tools/binman/test/312_capsule_missing_key.dts
> > > >  create mode 100644 tools/binman/test/313_capsule_missing_index.dts
> > > >  create mode 100644 tools/binman/test/314_capsule_missing_guid.dts
> > > >  create mode 100644 tools/binman/test/315_capsule_missing_payload.dts
> > > >
> >
> > [..]
> >
> > > > diff --git a/tools/binman/test/307_capsule.dts b/tools/binman/test/307_capsule.dts
> > > > new file mode 100644
> > > > index 0000000000..86552ff83f
> > > > --- /dev/null
> > > > +++ b/tools/binman/test/307_capsule.dts
> > > > @@ -0,0 +1,23 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +
> > > > +/dts-v1/;
> > > > +
> > > > +#include <sandbox_efi_capsule.h>
> > >
> > > We can't include sandbox files in binman! I Does it actually matter
> > > what the GUID value is? Can they all be the same? For now I think it
> > > is best to have
> > >
> > > #define BINMAN_TEST_GUID "xxx"
> > >
> > > repeated at the top of each .dts file. Hopefully at some point we can
> > > figure out a sensible way to provide a decoder ring for this mess, so
> > > we can use actually names in the binman definition instead of GUIDs.
> >
> > Oh, having thought about this a bit more, there is a much better way.
> >
> > In the entry type, allow the image_guid to be a string, like
> > 'binman-test' or 'sandbox-test'. Then binman can have a conversion
> > function to figure out the GUID to pass to the tool, e.g. in
> > efi_capsule.py:
> >
> > TYPE_TO_GUID = {
> >    'binman-test': '09123987987f98712',
> >    'sandbox-test':, '98123987123123987123987',
> > }
> >
> > def get_guid(type_str):
> >     'Get the GUID to use for a particular purpose"""
> >     return TYPE_TO_GUID.get(type_str, type_str)
> >
> > Then you can use these same strings in the sandbox tests, without
> > needing to include anything.
>
> While we can use this method for both sandbox and binman tests, I
> would prefer using the actual GUID strings for the sandbox tests. This
> will serve as an illustrative example to the user for how to pass the
> image GUID value for generating the capsule. For the binman tests, I
> can use this logic to avoid including the header file.

So long as it is easy, that's OK. But how does the user know which GUID to use?

Regards,
Simon
Sughosh Ganu Aug. 6, 2023, 7:27 p.m. UTC | #10 
hi Simon,

On Mon, 7 Aug 2023 at 00:16, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Sun, 6 Aug 2023 at 09:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Sat, 5 Aug 2023 at 22:50, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Sat, 5 Aug 2023 at 09:03, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > Add support in binman for generating EFI capsules. The capsule
> > > > > parameters can be specified through the capsule binman entry. Also add
> > > > > test cases in binman for testing capsule generation.
> > > > >
> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > ---
> > > > > Changes since V6:
> > > > > * Add macros for the GUID strings in sandbox_efi_capsule.h
> > > > > * Highlight that the private and public keys are mandatory for capsule
> > > > >   signing.
> > > > > * Add a URL link to the UEFI spec, as used in the rst files.
> > > > > * Use local vars for private and public keys in BuildSectionData()
> > > > > * Use local vars for input payload and capsule filenames in
> > > > >   BuildSectionData().
> > > > > * Drop the ProcessContents() and SetImagePos() as the superclass
> > > > >   functions suffice.
> > > > > * Use GUID macro names in the capsule test dts files.
> > > > > * Rename efi_capsule_payload.bin to capsule_input.bin.
> > > > >
> > > > >
> > > > >  include/sandbox_efi_capsule.h                 |  14 ++
> > > >
> > > > Please move this file to a later patch - see below.
> > > >
> > > > Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.
> > > >
> > > > >  tools/binman/entries.rst                      |  62 ++++++++
> > > > >  tools/binman/etype/efi_capsule.py             | 143 ++++++++++++++++++
> > > > >  tools/binman/ftest.py                         | 121 +++++++++++++++
> > > > >  tools/binman/test/307_capsule.dts             |  23 +++
> > > > >  tools/binman/test/308_capsule_signed.dts      |  25 +++
> > > > >  tools/binman/test/309_capsule_version.dts     |  24 +++
> > > > >  tools/binman/test/310_capsule_signed_ver.dts  |  26 ++++
> > > > >  tools/binman/test/311_capsule_oemflags.dts    |  24 +++
> > > > >  tools/binman/test/312_capsule_missing_key.dts |  24 +++
> > > > >  .../binman/test/313_capsule_missing_index.dts |  22 +++
> > > > >  .../binman/test/314_capsule_missing_guid.dts  |  19 +++
> > > > >  .../test/315_capsule_missing_payload.dts      |  19 +++
> > > > >  13 files changed, 546 insertions(+)
> > > > >  create mode 100644 include/sandbox_efi_capsule.h
> > > > >  create mode 100644 tools/binman/etype/efi_capsule.py
> > > > >  create mode 100644 tools/binman/test/307_capsule.dts
> > > > >  create mode 100644 tools/binman/test/308_capsule_signed.dts
> > > > >  create mode 100644 tools/binman/test/309_capsule_version.dts
> > > > >  create mode 100644 tools/binman/test/310_capsule_signed_ver.dts
> > > > >  create mode 100644 tools/binman/test/311_capsule_oemflags.dts
> > > > >  create mode 100644 tools/binman/test/312_capsule_missing_key.dts
> > > > >  create mode 100644 tools/binman/test/313_capsule_missing_index.dts
> > > > >  create mode 100644 tools/binman/test/314_capsule_missing_guid.dts
> > > > >  create mode 100644 tools/binman/test/315_capsule_missing_payload.dts
> > > > >
> > >
> > > [..]
> > >
> > > > > diff --git a/tools/binman/test/307_capsule.dts b/tools/binman/test/307_capsule.dts
> > > > > new file mode 100644
> > > > > index 0000000000..86552ff83f
> > > > > --- /dev/null
> > > > > +++ b/tools/binman/test/307_capsule.dts
> > > > > @@ -0,0 +1,23 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > +
> > > > > +/dts-v1/;
> > > > > +
> > > > > +#include <sandbox_efi_capsule.h>
> > > >
> > > > We can't include sandbox files in binman! I Does it actually matter
> > > > what the GUID value is? Can they all be the same? For now I think it
> > > > is best to have
> > > >
> > > > #define BINMAN_TEST_GUID "xxx"
> > > >
> > > > repeated at the top of each .dts file. Hopefully at some point we can
> > > > figure out a sensible way to provide a decoder ring for this mess, so
> > > > we can use actually names in the binman definition instead of GUIDs.
> > >
> > > Oh, having thought about this a bit more, there is a much better way.
> > >
> > > In the entry type, allow the image_guid to be a string, like
> > > 'binman-test' or 'sandbox-test'. Then binman can have a conversion
> > > function to figure out the GUID to pass to the tool, e.g. in
> > > efi_capsule.py:
> > >
> > > TYPE_TO_GUID = {
> > >    'binman-test': '09123987987f98712',
> > >    'sandbox-test':, '98123987123123987123987',
> > > }
> > >
> > > def get_guid(type_str):
> > >     'Get the GUID to use for a particular purpose"""
> > >     return TYPE_TO_GUID.get(type_str, type_str)
> > >
> > > Then you can use these same strings in the sandbox tests, without
> > > needing to include anything.
> >
> > While we can use this method for both sandbox and binman tests, I
> > would prefer using the actual GUID strings for the sandbox tests. This
> > will serve as an illustrative example to the user for how to pass the
> > image GUID value for generating the capsule. For the binman tests, I
> > can use this logic to avoid including the header file.
>
> So long as it is easy, that's OK. But how does the user know which GUID to use?

I think you got me wrong here. What I meant was that passing the GUID
value in the capsule entry like how it is currently done in the
u-boot.dtsi file would give the user an idea of how to specify the
GUID value for an image. If we use the string that you suggest above
for both binman tests as well as sandbox, the user might get confused
as to how the GUID value is to be passed in the normal scenario. I
want the sandbox capsule entries to also be kind of illustrative of
how a capsule entry would look like.

-sughosh





>
> Regards,
> Simon
Simon Glass Aug. 7, 2023, 1:33 a.m. UTC | #11 
Hi Sughosh,

On Sun, 6 Aug 2023 at 13:27, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Mon, 7 Aug 2023 at 00:16, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Sun, 6 Aug 2023 at 09:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Sat, 5 Aug 2023 at 22:50, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Sat, 5 Aug 2023 at 09:03, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > >
> > > > > > Add support in binman for generating EFI capsules. The capsule
> > > > > > parameters can be specified through the capsule binman entry. Also add
> > > > > > test cases in binman for testing capsule generation.
> > > > > >
> > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > ---
> > > > > > Changes since V6:
> > > > > > * Add macros for the GUID strings in sandbox_efi_capsule.h
> > > > > > * Highlight that the private and public keys are mandatory for capsule
> > > > > >   signing.
> > > > > > * Add a URL link to the UEFI spec, as used in the rst files.
> > > > > > * Use local vars for private and public keys in BuildSectionData()
> > > > > > * Use local vars for input payload and capsule filenames in
> > > > > >   BuildSectionData().
> > > > > > * Drop the ProcessContents() and SetImagePos() as the superclass
> > > > > >   functions suffice.
> > > > > > * Use GUID macro names in the capsule test dts files.
> > > > > > * Rename efi_capsule_payload.bin to capsule_input.bin.
> > > > > >
> > > > > >
> > > > > >  include/sandbox_efi_capsule.h                 |  14 ++
> > > > >
> > > > > Please move this file to a later patch - see below.
> > > > >
> > > > > Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.
> > > > >
> > > > > >  tools/binman/entries.rst                      |  62 ++++++++
> > > > > >  tools/binman/etype/efi_capsule.py             | 143 ++++++++++++++++++
> > > > > >  tools/binman/ftest.py                         | 121 +++++++++++++++
> > > > > >  tools/binman/test/307_capsule.dts             |  23 +++
> > > > > >  tools/binman/test/308_capsule_signed.dts      |  25 +++
> > > > > >  tools/binman/test/309_capsule_version.dts     |  24 +++
> > > > > >  tools/binman/test/310_capsule_signed_ver.dts  |  26 ++++
> > > > > >  tools/binman/test/311_capsule_oemflags.dts    |  24 +++
> > > > > >  tools/binman/test/312_capsule_missing_key.dts |  24 +++
> > > > > >  .../binman/test/313_capsule_missing_index.dts |  22 +++
> > > > > >  .../binman/test/314_capsule_missing_guid.dts  |  19 +++
> > > > > >  .../test/315_capsule_missing_payload.dts      |  19 +++
> > > > > >  13 files changed, 546 insertions(+)
> > > > > >  create mode 100644 include/sandbox_efi_capsule.h
> > > > > >  create mode 100644 tools/binman/etype/efi_capsule.py
> > > > > >  create mode 100644 tools/binman/test/307_capsule.dts
> > > > > >  create mode 100644 tools/binman/test/308_capsule_signed.dts
> > > > > >  create mode 100644 tools/binman/test/309_capsule_version.dts
> > > > > >  create mode 100644 tools/binman/test/310_capsule_signed_ver.dts
> > > > > >  create mode 100644 tools/binman/test/311_capsule_oemflags.dts
> > > > > >  create mode 100644 tools/binman/test/312_capsule_missing_key.dts
> > > > > >  create mode 100644 tools/binman/test/313_capsule_missing_index.dts
> > > > > >  create mode 100644 tools/binman/test/314_capsule_missing_guid.dts
> > > > > >  create mode 100644 tools/binman/test/315_capsule_missing_payload.dts
> > > > > >
> > > >
> > > > [..]
> > > >
> > > > > > diff --git a/tools/binman/test/307_capsule.dts b/tools/binman/test/307_capsule.dts
> > > > > > new file mode 100644
> > > > > > index 0000000000..86552ff83f
> > > > > > --- /dev/null
> > > > > > +++ b/tools/binman/test/307_capsule.dts
> > > > > > @@ -0,0 +1,23 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > > +
> > > > > > +/dts-v1/;
> > > > > > +
> > > > > > +#include <sandbox_efi_capsule.h>
> > > > >
> > > > > We can't include sandbox files in binman! I Does it actually matter
> > > > > what the GUID value is? Can they all be the same? For now I think it
> > > > > is best to have
> > > > >
> > > > > #define BINMAN_TEST_GUID "xxx"
> > > > >
> > > > > repeated at the top of each .dts file. Hopefully at some point we can
> > > > > figure out a sensible way to provide a decoder ring for this mess, so
> > > > > we can use actually names in the binman definition instead of GUIDs.
> > > >
> > > > Oh, having thought about this a bit more, there is a much better way.
> > > >
> > > > In the entry type, allow the image_guid to be a string, like
> > > > 'binman-test' or 'sandbox-test'. Then binman can have a conversion
> > > > function to figure out the GUID to pass to the tool, e.g. in
> > > > efi_capsule.py:
> > > >
> > > > TYPE_TO_GUID = {
> > > >    'binman-test': '09123987987f98712',
> > > >    'sandbox-test':, '98123987123123987123987',
> > > > }
> > > >
> > > > def get_guid(type_str):
> > > >     'Get the GUID to use for a particular purpose"""
> > > >     return TYPE_TO_GUID.get(type_str, type_str)
> > > >
> > > > Then you can use these same strings in the sandbox tests, without
> > > > needing to include anything.
> > >
> > > While we can use this method for both sandbox and binman tests, I
> > > would prefer using the actual GUID strings for the sandbox tests. This
> > > will serve as an illustrative example to the user for how to pass the
> > > image GUID value for generating the capsule. For the binman tests, I
> > > can use this logic to avoid including the header file.
> >
> > So long as it is easy, that's OK. But how does the user know which GUID to use?
>
> I think you got me wrong here. What I meant was that passing the GUID
> value in the capsule entry like how it is currently done in the
> u-boot.dtsi file would give the user an idea of how to specify the
> GUID value for an image. If we use the string that you suggest above
> for both binman tests as well as sandbox, the user might get confused
> as to how the GUID value is to be passed in the normal scenario. I
> want the sandbox capsule entries to also be kind of illustrative of
> how a capsule entry would look like.

OK I see.

Of course, I would like to see a decoder ring for GUIDs somewhere.
Perhaps Binman is a good place for that, or a file it can read?

Regards,
Simon
Sughosh Ganu Aug. 7, 2023, 6:40 a.m. UTC | #12 
hi Simon,

On Mon, 7 Aug 2023 at 07:04, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Sun, 6 Aug 2023 at 13:27, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Mon, 7 Aug 2023 at 00:16, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Sun, 6 Aug 2023 at 09:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Sat, 5 Aug 2023 at 22:50, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Sat, 5 Aug 2023 at 09:03, Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > >
> > > > > > > Add support in binman for generating EFI capsules. The capsule
> > > > > > > parameters can be specified through the capsule binman entry. Also add
> > > > > > > test cases in binman for testing capsule generation.
> > > > > > >
> > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > > ---
> > > > > > > Changes since V6:
> > > > > > > * Add macros for the GUID strings in sandbox_efi_capsule.h
> > > > > > > * Highlight that the private and public keys are mandatory for capsule
> > > > > > >   signing.
> > > > > > > * Add a URL link to the UEFI spec, as used in the rst files.
> > > > > > > * Use local vars for private and public keys in BuildSectionData()
> > > > > > > * Use local vars for input payload and capsule filenames in
> > > > > > >   BuildSectionData().
> > > > > > > * Drop the ProcessContents() and SetImagePos() as the superclass
> > > > > > >   functions suffice.
> > > > > > > * Use GUID macro names in the capsule test dts files.
> > > > > > > * Rename efi_capsule_payload.bin to capsule_input.bin.
> > > > > > >
> > > > > > >
> > > > > > >  include/sandbox_efi_capsule.h                 |  14 ++
> > > > > >
> > > > > > Please move this file to a later patch - see below.
> > > > > >
> > > > > > Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.
> > > > > >
> > > > > > >  tools/binman/entries.rst                      |  62 ++++++++
> > > > > > >  tools/binman/etype/efi_capsule.py             | 143 ++++++++++++++++++
> > > > > > >  tools/binman/ftest.py                         | 121 +++++++++++++++
> > > > > > >  tools/binman/test/307_capsule.dts             |  23 +++
> > > > > > >  tools/binman/test/308_capsule_signed.dts      |  25 +++
> > > > > > >  tools/binman/test/309_capsule_version.dts     |  24 +++
> > > > > > >  tools/binman/test/310_capsule_signed_ver.dts  |  26 ++++
> > > > > > >  tools/binman/test/311_capsule_oemflags.dts    |  24 +++
> > > > > > >  tools/binman/test/312_capsule_missing_key.dts |  24 +++
> > > > > > >  .../binman/test/313_capsule_missing_index.dts |  22 +++
> > > > > > >  .../binman/test/314_capsule_missing_guid.dts  |  19 +++
> > > > > > >  .../test/315_capsule_missing_payload.dts      |  19 +++
> > > > > > >  13 files changed, 546 insertions(+)
> > > > > > >  create mode 100644 include/sandbox_efi_capsule.h
> > > > > > >  create mode 100644 tools/binman/etype/efi_capsule.py
> > > > > > >  create mode 100644 tools/binman/test/307_capsule.dts
> > > > > > >  create mode 100644 tools/binman/test/308_capsule_signed.dts
> > > > > > >  create mode 100644 tools/binman/test/309_capsule_version.dts
> > > > > > >  create mode 100644 tools/binman/test/310_capsule_signed_ver.dts
> > > > > > >  create mode 100644 tools/binman/test/311_capsule_oemflags.dts
> > > > > > >  create mode 100644 tools/binman/test/312_capsule_missing_key.dts
> > > > > > >  create mode 100644 tools/binman/test/313_capsule_missing_index.dts
> > > > > > >  create mode 100644 tools/binman/test/314_capsule_missing_guid.dts
> > > > > > >  create mode 100644 tools/binman/test/315_capsule_missing_payload.dts
> > > > > > >
> > > > >
> > > > > [..]
> > > > >
> > > > > > > diff --git a/tools/binman/test/307_capsule.dts b/tools/binman/test/307_capsule.dts
> > > > > > > new file mode 100644
> > > > > > > index 0000000000..86552ff83f
> > > > > > > --- /dev/null
> > > > > > > +++ b/tools/binman/test/307_capsule.dts
> > > > > > > @@ -0,0 +1,23 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > > > +
> > > > > > > +/dts-v1/;
> > > > > > > +
> > > > > > > +#include <sandbox_efi_capsule.h>
> > > > > >
> > > > > > We can't include sandbox files in binman! I Does it actually matter
> > > > > > what the GUID value is? Can they all be the same? For now I think it
> > > > > > is best to have
> > > > > >
> > > > > > #define BINMAN_TEST_GUID "xxx"
> > > > > >
> > > > > > repeated at the top of each .dts file. Hopefully at some point we can
> > > > > > figure out a sensible way to provide a decoder ring for this mess, so
> > > > > > we can use actually names in the binman definition instead of GUIDs.
> > > > >
> > > > > Oh, having thought about this a bit more, there is a much better way.
> > > > >
> > > > > In the entry type, allow the image_guid to be a string, like
> > > > > 'binman-test' or 'sandbox-test'. Then binman can have a conversion
> > > > > function to figure out the GUID to pass to the tool, e.g. in
> > > > > efi_capsule.py:
> > > > >
> > > > > TYPE_TO_GUID = {
> > > > >    'binman-test': '09123987987f98712',
> > > > >    'sandbox-test':, '98123987123123987123987',
> > > > > }
> > > > >
> > > > > def get_guid(type_str):
> > > > >     'Get the GUID to use for a particular purpose"""
> > > > >     return TYPE_TO_GUID.get(type_str, type_str)
> > > > >
> > > > > Then you can use these same strings in the sandbox tests, without
> > > > > needing to include anything.
> > > >
> > > > While we can use this method for both sandbox and binman tests, I
> > > > would prefer using the actual GUID strings for the sandbox tests. This
> > > > will serve as an illustrative example to the user for how to pass the
> > > > image GUID value for generating the capsule. For the binman tests, I
> > > > can use this logic to avoid including the header file.
> > >
> > > So long as it is easy, that's OK. But how does the user know which GUID to use?
> >
> > I think you got me wrong here. What I meant was that passing the GUID
> > value in the capsule entry like how it is currently done in the
> > u-boot.dtsi file would give the user an idea of how to specify the
> > GUID value for an image. If we use the string that you suggest above
> > for both binman tests as well as sandbox, the user might get confused
> > as to how the GUID value is to be passed in the normal scenario. I
> > want the sandbox capsule entries to also be kind of illustrative of
> > how a capsule entry would look like.
>
> OK I see.
>
> Of course, I would like to see a decoder ring for GUIDs somewhere.
> Perhaps Binman is a good place for that, or a file it can read?

I'd suggest we start with your above solution for the binman tests. If
we get consensus amongst the folks who work on EFI to have such a GUID
decoder file, it can be worked upon later. I think these are things
which are part of a larger point that you have about specifying GUIDs
in a particular way, and that should be tackled separately, and not
through this series, especially at this stage in the series. My 2
cents.

-sughosh
Simon Glass Aug. 7, 2023, 3:11 p.m. UTC | #13 
Hi Sughosh,

On Mon, 7 Aug 2023 at 00:40, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Mon, 7 Aug 2023 at 07:04, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Sun, 6 Aug 2023 at 13:27, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Mon, 7 Aug 2023 at 00:16, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Sun, 6 Aug 2023 at 09:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > hi Simon,
> > > > >
> > > > > On Sat, 5 Aug 2023 at 22:50, Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Sat, 5 Aug 2023 at 09:03, Simon Glass <sjg@chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Sughosh,
> > > > > > >
> > > > > > > On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > > >
> > > > > > > > Add support in binman for generating EFI capsules. The capsule
> > > > > > > > parameters can be specified through the capsule binman entry. Also add
> > > > > > > > test cases in binman for testing capsule generation.
> > > > > > > >
> > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > > > ---
> > > > > > > > Changes since V6:
> > > > > > > > * Add macros for the GUID strings in sandbox_efi_capsule.h
> > > > > > > > * Highlight that the private and public keys are mandatory for capsule
> > > > > > > >   signing.
> > > > > > > > * Add a URL link to the UEFI spec, as used in the rst files.
> > > > > > > > * Use local vars for private and public keys in BuildSectionData()
> > > > > > > > * Use local vars for input payload and capsule filenames in
> > > > > > > >   BuildSectionData().
> > > > > > > > * Drop the ProcessContents() and SetImagePos() as the superclass
> > > > > > > >   functions suffice.
> > > > > > > > * Use GUID macro names in the capsule test dts files.
> > > > > > > > * Rename efi_capsule_payload.bin to capsule_input.bin.
> > > > > > > >
> > > > > > > >
> > > > > > > >  include/sandbox_efi_capsule.h                 |  14 ++
> > > > > > >
> > > > > > > Please move this file to a later patch - see below.
> > > > > > >
> > > > > > > Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.
> > > > > > >
> > > > > > > >  tools/binman/entries.rst                      |  62 ++++++++
> > > > > > > >  tools/binman/etype/efi_capsule.py             | 143 ++++++++++++++++++
> > > > > > > >  tools/binman/ftest.py                         | 121 +++++++++++++++
> > > > > > > >  tools/binman/test/307_capsule.dts             |  23 +++
> > > > > > > >  tools/binman/test/308_capsule_signed.dts      |  25 +++
> > > > > > > >  tools/binman/test/309_capsule_version.dts     |  24 +++
> > > > > > > >  tools/binman/test/310_capsule_signed_ver.dts  |  26 ++++
> > > > > > > >  tools/binman/test/311_capsule_oemflags.dts    |  24 +++
> > > > > > > >  tools/binman/test/312_capsule_missing_key.dts |  24 +++
> > > > > > > >  .../binman/test/313_capsule_missing_index.dts |  22 +++
> > > > > > > >  .../binman/test/314_capsule_missing_guid.dts  |  19 +++
> > > > > > > >  .../test/315_capsule_missing_payload.dts      |  19 +++
> > > > > > > >  13 files changed, 546 insertions(+)
> > > > > > > >  create mode 100644 include/sandbox_efi_capsule.h
> > > > > > > >  create mode 100644 tools/binman/etype/efi_capsule.py
> > > > > > > >  create mode 100644 tools/binman/test/307_capsule.dts
> > > > > > > >  create mode 100644 tools/binman/test/308_capsule_signed.dts
> > > > > > > >  create mode 100644 tools/binman/test/309_capsule_version.dts
> > > > > > > >  create mode 100644 tools/binman/test/310_capsule_signed_ver.dts
> > > > > > > >  create mode 100644 tools/binman/test/311_capsule_oemflags.dts
> > > > > > > >  create mode 100644 tools/binman/test/312_capsule_missing_key.dts
> > > > > > > >  create mode 100644 tools/binman/test/313_capsule_missing_index.dts
> > > > > > > >  create mode 100644 tools/binman/test/314_capsule_missing_guid.dts
> > > > > > > >  create mode 100644 tools/binman/test/315_capsule_missing_payload.dts
> > > > > > > >
> > > > > >
> > > > > > [..]
> > > > > >
> > > > > > > > diff --git a/tools/binman/test/307_capsule.dts b/tools/binman/test/307_capsule.dts
> > > > > > > > new file mode 100644
> > > > > > > > index 0000000000..86552ff83f
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/tools/binman/test/307_capsule.dts
> > > > > > > > @@ -0,0 +1,23 @@
> > > > > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > > > > +
> > > > > > > > +/dts-v1/;
> > > > > > > > +
> > > > > > > > +#include <sandbox_efi_capsule.h>
> > > > > > >
> > > > > > > We can't include sandbox files in binman! I Does it actually matter
> > > > > > > what the GUID value is? Can they all be the same? For now I think it
> > > > > > > is best to have
> > > > > > >
> > > > > > > #define BINMAN_TEST_GUID "xxx"
> > > > > > >
> > > > > > > repeated at the top of each .dts file. Hopefully at some point we can
> > > > > > > figure out a sensible way to provide a decoder ring for this mess, so
> > > > > > > we can use actually names in the binman definition instead of GUIDs.
> > > > > >
> > > > > > Oh, having thought about this a bit more, there is a much better way.
> > > > > >
> > > > > > In the entry type, allow the image_guid to be a string, like
> > > > > > 'binman-test' or 'sandbox-test'. Then binman can have a conversion
> > > > > > function to figure out the GUID to pass to the tool, e.g. in
> > > > > > efi_capsule.py:
> > > > > >
> > > > > > TYPE_TO_GUID = {
> > > > > >    'binman-test': '09123987987f98712',
> > > > > >    'sandbox-test':, '98123987123123987123987',
> > > > > > }
> > > > > >
> > > > > > def get_guid(type_str):
> > > > > >     'Get the GUID to use for a particular purpose"""
> > > > > >     return TYPE_TO_GUID.get(type_str, type_str)
> > > > > >
> > > > > > Then you can use these same strings in the sandbox tests, without
> > > > > > needing to include anything.
> > > > >
> > > > > While we can use this method for both sandbox and binman tests, I
> > > > > would prefer using the actual GUID strings for the sandbox tests. This
> > > > > will serve as an illustrative example to the user for how to pass the
> > > > > image GUID value for generating the capsule. For the binman tests, I
> > > > > can use this logic to avoid including the header file.
> > > >
> > > > So long as it is easy, that's OK. But how does the user know which GUID to use?
> > >
> > > I think you got me wrong here. What I meant was that passing the GUID
> > > value in the capsule entry like how it is currently done in the
> > > u-boot.dtsi file would give the user an idea of how to specify the
> > > GUID value for an image. If we use the string that you suggest above
> > > for both binman tests as well as sandbox, the user might get confused
> > > as to how the GUID value is to be passed in the normal scenario. I
> > > want the sandbox capsule entries to also be kind of illustrative of
> > > how a capsule entry would look like.
> >
> > OK I see.
> >
> > Of course, I would like to see a decoder ring for GUIDs somewhere.
> > Perhaps Binman is a good place for that, or a file it can read?
>
> I'd suggest we start with your above solution for the binman tests. If
> we get consensus amongst the folks who work on EFI to have such a GUID
> decoder file, it can be worked upon later. I think these are things
> which are part of a larger point that you have about specifying GUIDs
> in a particular way, and that should be tackled separately, and not
> through this series, especially at this stage in the series. My 2
> cents.

That sounds OK to me. When we get to it, I think we should create a
database of GUIDs, with a name for each, and perhaps an ID number, so
we can allocate them.

Regards,
Simon
diff mbox series

Patch

diff --git a/include/sandbox_efi_capsule.h b/include/sandbox_efi_capsule.h
new file mode 100644
index 0000000000..da71b87a18
--- /dev/null
+++ b/include/sandbox_efi_capsule.h
@@ -0,0 +1,14 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2023, Linaro Limited
+ */
+
+#if !defined(_SANDBOX_EFI_CAPSULE_H_)
+#define _SANDBOX_EFI_CAPSULE_H_
+
+#define SANDBOX_UBOOT_IMAGE_GUID	"09d7cf52-0720-4710-91d1-08469b7fe9c8"
+#define SANDBOX_UBOOT_ENV_IMAGE_GUID	"5a7021f5-fef2-48b4-aaba-832e777418c0"
+#define SANDBOX_FIT_IMAGE_GUID		"3673b45d-6a7c-46f3-9e60-adabb03f7937"
+#define SANDBOX_INCORRECT_GUID		"058b7d83-50d5-4c47-a195-60d86ad341c4"
+
+#endif /* _SANDBOX_EFI_CAPSULE_H_ */
diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
index f2376932be..fc094b9c08 100644
--- a/tools/binman/entries.rst
+++ b/tools/binman/entries.rst
@@ -468,6 +468,68 @@  updating the EC on startup via software sync.
 
 
 
+.. _etype_efi_capsule:
+
+Entry: capsule: Entry for generating EFI Capsule files
+------------------------------------------------------
+
+The parameters needed for generation of the capsules can
+be provided as properties in the entry.
+
+Properties / Entry arguments:
+    - image-index: Unique number for identifying corresponding
+      payload image. Number between 1 and descriptor count, i.e.
+      the total number of firmware images that can be updated.
+    - image-type-id: Image GUID which will be used for identifying the
+      updatable image on the board.
+    - hardware-instance: Optional number for identifying unique
+      hardware instance of a device in the system. Default value of 0
+      for images where value is not to be used.
+    - fw-version: Optional value of image version that can be put on
+      the capsule through the Firmware Management Protocol(FMP) header.
+    - monotonic-count: Count used when signing an image.
+    - private-key: Path to PEM formatted .key private key file. This property
+      is mandatory for generating signed capsules.
+    - public-key-cert: Path to PEM formatted .crt public key certificate
+      file. This property is mandatory for generating signed capsules.
+    - oem-flags - Optional OEM flags to be passed through capsule header.
+
+    Since this is a subclass of Entry_section, all properties of the parent
+    class also apply here.
+
+For more details on the description of the capsule format, and the capsule
+update functionality, refer Section 8.5 and Chapter 23 in the `UEFI
+specification`_.
+
+The capsule parameters like image index and image GUID are passed as
+properties in the entry. The payload to be used in the capsule is to be
+provided as a subnode of the capsule entry.
+
+A typical capsule entry node would then look something like this::
+
+    capsule {
+            type = "efi-capsule";
+            image-index = <0x1>;
+            /* Image GUID for testing capsule update */
+            image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
+            hardware-instance = <0x0>;
+            private-key = "path/to/the/private/key";
+            public-key-cert = "path/to/the/public-key-cert";
+            oem-flags = <0x8000>;
+
+            u-boot {
+            };
+    };
+
+In the above example, the capsule payload is the U-Boot image. The
+capsule entry would read the contents of the payload and put them
+into the capsule. Any external file can also be specified as the
+payload using the blob-ext subnode.
+
+.. _`UEFI specification`: https://uefi.org/sites/default/files/resources/UEFI_Spec_2_10_Aug29.pdf
+
+
+
 .. _etype_encrypted:
 
 Entry: encrypted: Externally built encrypted binary blob
diff --git a/tools/binman/etype/efi_capsule.py b/tools/binman/etype/efi_capsule.py
new file mode 100644
index 0000000000..bfdca94e26
--- /dev/null
+++ b/tools/binman/etype/efi_capsule.py
@@ -0,0 +1,143 @@ 
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (c) 2023 Linaro Limited
+#
+# Entry-type module for producing a EFI capsule
+#
+
+import os
+
+from binman.entry import Entry
+from binman.etype.section import Entry_section
+from dtoc import fdt_util
+from u_boot_pylib import tools
+
+class Entry_efi_capsule(Entry_section):
+    """Generate EFI capsules
+
+    The parameters needed for generation of the capsules can
+    be provided as properties in the entry.
+
+    Properties / Entry arguments:
+    - image-index: Unique number for identifying corresponding
+      payload image. Number between 1 and descriptor count, i.e.
+      the total number of firmware images that can be updated.
+    - image-type-id: Image GUID which will be used for identifying the
+      updatable image on the board.
+    - hardware-instance: Optional number for identifying unique
+      hardware instance of a device in the system. Default value of 0
+      for images where value is not to be used.
+    - fw-version: Optional value of image version that can be put on
+      the capsule through the Firmware Management Protocol(FMP) header.
+    - monotonic-count: Count used when signing an image.
+    - private-key: Path to PEM formatted .key private key file. This property
+      is mandatory for generating signed capsules.
+    - public-key-cert: Path to PEM formatted .crt public key certificate
+      file. This property is mandatory for generating signed capsules.
+    - oem-flags - Optional OEM flags to be passed through capsule header.
+
+    Since this is a subclass of Entry_section, all properties of the parent
+    class also apply here.
+
+    For more details on the description of the capsule format, and the capsule
+    update functionality, refer Section 8.5 and Chapter 23 in the `UEFI
+    specification`_.
+
+    The capsule parameters like image index and image GUID are passed as
+    properties in the entry. The payload to be used in the capsule is to be
+    provided as a subnode of the capsule entry.
+
+    A typical capsule entry node would then look something like this
+
+    capsule {
+            type = "efi-capsule";
+            image-index = <0x1>;
+            /* Image GUID for testing capsule update */
+            image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
+            hardware-instance = <0x0>;
+            private-key = "path/to/the/private/key";
+            public-key-cert = "path/to/the/public-key-cert";
+            oem-flags = <0x8000>;
+
+            u-boot {
+            };
+    };
+
+    In the above example, the capsule payload is the U-Boot image. The
+    capsule entry would read the contents of the payload and put them
+    into the capsule. Any external file can also be specified as the
+    payload using the blob-ext subnode.
+
+    .. _`UEFI specification`: https://uefi.org/sites/default/files/resources/UEFI_Spec_2_10_Aug29.pdf
+    """
+    def __init__(self, section, etype, node):
+        super().__init__(section, etype, node)
+        self.required_props = ['image-index', 'image-type-id']
+        self.image_index = 0
+        self.image_guid = ''
+        self.hardware_instance = 0
+        self.monotonic_count = 0
+        self.fw_version = 0
+        self.oem_flags = 0
+        self.private_key = ''
+        self.public_key_cert = ''
+        self.auth = 0
+
+    def ReadNode(self):
+        self.ReadEntries()
+        super().ReadNode()
+
+        self.image_index = fdt_util.GetInt(self._node, 'image-index')
+        self.image_guid = fdt_util.GetString(self._node, 'image-type-id')
+        self.fw_version = fdt_util.GetInt(self._node, 'fw-version')
+        self.hardware_instance = fdt_util.GetInt(self._node, 'hardware-instance')
+        self.monotonic_count = fdt_util.GetInt(self._node, 'monotonic-count')
+        self.oem_flags = fdt_util.GetInt(self._node, 'oem-flags')
+
+        self.private_key = fdt_util.GetString(self._node, 'private-key')
+        self.public_key_cert = fdt_util.GetString(self._node, 'public-key-cert')
+        if ((self.private_key and not self.public_key_cert) or (self.public_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.public_key_cert):
+            self.auth = 0
+        else:
+            self.auth = 1
+
+    def ReadEntries(self):
+        """Read the subnode to get the payload for this capsule"""
+        # We can have a single payload per capsule
+        if len(self._node.subnodes) == 0:
+            self.Raise('The capsule entry expects at least one subnode for payload')
+
+        for node in self._node.subnodes:
+            entry = Entry.Create(self, node)
+            entry.ReadNode()
+            self._entries[entry.name] = entry
+
+    def BuildSectionData(self, required):
+        private_key = ''
+        public_key_cert = ''
+        if self.auth:
+            if not os.path.isabs(self.private_key):
+                private_key =  tools.get_input_filename(self.private_key)
+            if not os.path.isabs(self.public_key_cert):
+                public_key_cert = tools.get_input_filename(self.public_key_cert)
+        data, payload, _ = self.collect_contents_to_file(
+            self._entries.values(), 'capsule_payload')
+        outfile = self._filename if self._filename else self._node.name
+        capsule_fname = tools.get_output_filename(outfile)
+        ret = self.mkeficapsule.generate_capsule(self.image_index,
+                                                  self.image_guid,
+                                                  self.hardware_instance,
+                                                  payload,
+                                                  capsule_fname,
+                                                  private_key,
+                                                  public_key_cert,
+                                                  self.monotonic_count,
+                                                  self.fw_version,
+                                                  self.oem_flags)
+        if ret is not None:
+            os.remove(payload)
+            return tools.read_file(capsule_fname)
+
+    def AddBintools(self, btools):
+        self.mkeficapsule = self.AddBintool(btools, 'mkeficapsule')
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 36428ec343..9bda0157f7 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -48,6 +48,7 @@  U_BOOT_VPL_DATA       = b'vpl76543210fedcbazywxyz_'
 BLOB_DATA             = b'89'
 ME_DATA               = b'0abcd'
 VGA_DATA              = b'vga'
+EFI_CAPSULE_DATA      = b'efi'
 U_BOOT_DTB_DATA       = b'udtb'
 U_BOOT_SPL_DTB_DATA   = b'spldtb'
 U_BOOT_TPL_DTB_DATA   = b'tpldtb'
@@ -119,6 +120,11 @@  COMP_BINTOOLS = ['bzip2', 'gzip', 'lz4', 'lzma_alone', 'lzop', 'xz', 'zstd']
 
 TEE_ADDR = 0x5678
 
+# Firmware Management Protocol(FMP) GUID
+FW_MGMT_GUID = 'edd5cb6d2de8444cbda17194199ad92a'
+# Image GUID specified in the DTS
+CAPSULE_IMAGE_GUID = '52cfd7092007104791d108469b7fe9c8'
+
 class TestFunctional(unittest.TestCase):
     """Functional tests for binman
 
@@ -215,6 +221,7 @@  class TestFunctional(unittest.TestCase):
         TestFunctional._MakeInputFile('scp.bin', SCP_DATA)
         TestFunctional._MakeInputFile('rockchip-tpl.bin', ROCKCHIP_TPL_DATA)
         TestFunctional._MakeInputFile('ti_unsecure.bin', TI_UNSECURE_DATA)
+        TestFunctional._MakeInputFile('capsule_input.bin', EFI_CAPSULE_DATA)
 
         # Add a few .dtb files for testing
         TestFunctional._MakeInputFile('%s/test-fdt1.dtb' % TEST_FDT_SUBDIR,
@@ -7139,5 +7146,119 @@  fdt         fdtmap                Extract the devicetree blob from the fdtmap
          self.assertEqual(fdt_util.GetString(key_node, "key-name-hint"),
                           "key")
 
+    def _CheckCapsule(self, data, signed_capsule=False, version_check=False,
+                      capoemflags=False):
+        fmp_signature = "4d535331" # 'M', 'S', 'S', '1'
+        fmp_size = "10"
+        fmp_fw_version = "02"
+        oemflag = "0080"
+
+        payload_data = EFI_CAPSULE_DATA
+
+        # Firmware Management Protocol(FMP) GUID - offset(0 - 32)
+        self.assertEqual(FW_MGMT_GUID, data.hex()[:32])
+        # Image GUID - offset(96 - 128)
+        self.assertEqual(CAPSULE_IMAGE_GUID, data.hex()[96:128])
+
+        if capoemflags:
+            # OEM Flags - offset(40 - 44)
+            self.assertEqual(oemflag, data.hex()[40:44])
+        if signed_capsule and version_check:
+            # FMP header signature - offset(4770 - 4778)
+            self.assertEqual(fmp_signature, data.hex()[4770:4778])
+            # FMP header size - offset(4778 - 4780)
+            self.assertEqual(fmp_size, data.hex()[4778:4780])
+            # firmware version - offset(4786 - 4788)
+            self.assertEqual(fmp_fw_version, data.hex()[4786:4788])
+            # payload offset signed capsule(4802 - 4808)
+            self.assertEqual(payload_data.hex(), data.hex()[4802:4808])
+        elif signed_capsule:
+            # payload offset signed capsule(4770 - 4776)
+            self.assertEqual(payload_data.hex(), data.hex()[4770:4776])
+        elif version_check:
+            # FMP header signature - offset(184 - 192)
+            self.assertEqual(fmp_signature, data.hex()[184:192])
+            # FMP header size - offset(192 - 194)
+            self.assertEqual(fmp_size, data.hex()[192:194])
+            # firmware version - offset(200 - 202)
+            self.assertEqual(fmp_fw_version, data.hex()[200:202])
+            # payload offset for non-signed capsule with version header(216 - 222)
+            self.assertEqual(payload_data.hex(), data.hex()[216:222])
+        else:
+            # payload offset for non-signed capsule with no version header(184 - 190)
+            self.assertEqual(payload_data.hex(), data.hex()[184:190])
+
+    def testCapsuleGen(self):
+        """Test generation of EFI capsule"""
+        data = self._DoReadFile('307_capsule.dts')
+
+        self._CheckCapsule(data)
+
+    def testSignedCapsuleGen(self):
+        """Test generation of EFI capsule"""
+        data = tools.read_file(self.TestFile("key.key"))
+        self._MakeInputFile("key.key", data)
+        data = tools.read_file(self.TestFile("key.pem"))
+        self._MakeInputFile("key.crt", data)
+
+        data = self._DoReadFile('308_capsule_signed.dts')
+
+        self._CheckCapsule(data, signed_capsule=True)
+
+    def testCapsuleGenVersionSupport(self):
+        """Test generation of EFI capsule with version support"""
+        data = self._DoReadFile('309_capsule_version.dts')
+
+        self._CheckCapsule(data, version_check=True)
+
+    def testCapsuleGenSignedVer(self):
+        """Test generation of signed EFI capsule with version information"""
+        data = tools.read_file(self.TestFile("key.key"))
+        self._MakeInputFile("key.key", data)
+        data = tools.read_file(self.TestFile("key.pem"))
+        self._MakeInputFile("key.crt", data)
+
+        data = self._DoReadFile('310_capsule_signed_ver.dts')
+
+        self._CheckCapsule(data, signed_capsule=True, version_check=True)
+
+    def testCapsuleGenCapOemFlags(self):
+        """Test generation of EFI capsule with OEM Flags set"""
+        data = self._DoReadFile('311_capsule_oemflags.dts')
+
+        self._CheckCapsule(data, capoemflags=True)
+
+    def testCapsuleGenKeyMissing(self):
+        """Test that binman errors out on missing key"""
+        with self.assertRaises(ValueError) as e:
+            self._DoReadFile('312_capsule_missing_key.dts')
+
+        self.assertIn("Both private key and public key certificate need to be provided",
+                      str(e.exception))
+
+    def testCapsuleGenIndexMissing(self):
+        """Test that binman errors out on missing image index"""
+        with self.assertRaises(ValueError) as e:
+            self._DoReadFile('313_capsule_missing_index.dts')
+
+        self.assertIn("entry is missing properties: image-index",
+                      str(e.exception))
+
+    def testCapsuleGenGuidMissing(self):
+        """Test that binman errors out on missing image GUID"""
+        with self.assertRaises(ValueError) as e:
+            self._DoReadFile('314_capsule_missing_guid.dts')
+
+        self.assertIn("entry is missing properties: image-type-id",
+                      str(e.exception))
+
+    def testCapsuleGenPayloadMissing(self):
+        """Test that binman errors out on missing input(payload)image"""
+        with self.assertRaises(ValueError) as e:
+            self._DoReadFile('315_capsule_missing_payload.dts')
+
+        self.assertIn("The capsule entry expects at least one subnode for payload",
+                      str(e.exception))
+
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/test/307_capsule.dts b/tools/binman/test/307_capsule.dts
new file mode 100644
index 0000000000..86552ff83f
--- /dev/null
+++ b/tools/binman/test/307_capsule.dts
@@ -0,0 +1,23 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+#include <sandbox_efi_capsule.h>
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		efi-capsule {
+			image-index = <0x1>;
+			/* Image GUID for testing capsule update */
+			image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
+			hardware-instance = <0x0>;
+
+			blob {
+				filename = "capsule_input.bin";
+			};
+		};
+	};
+};
diff --git a/tools/binman/test/308_capsule_signed.dts b/tools/binman/test/308_capsule_signed.dts
new file mode 100644
index 0000000000..37d2f35e71
--- /dev/null
+++ b/tools/binman/test/308_capsule_signed.dts
@@ -0,0 +1,25 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+#include <sandbox_efi_capsule.h>
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		efi-capsule {
+			image-index = <0x1>;
+			/* Image GUID for testing capsule update */
+			image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
+			hardware-instance = <0x0>;
+			private-key = "key.key";
+			public-key-cert = "key.crt";
+
+			blob {
+				filename = "capsule_input.bin";
+			};
+		};
+	};
+};
diff --git a/tools/binman/test/309_capsule_version.dts b/tools/binman/test/309_capsule_version.dts
new file mode 100644
index 0000000000..a3d2f96f5c
--- /dev/null
+++ b/tools/binman/test/309_capsule_version.dts
@@ -0,0 +1,24 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+#include <sandbox_efi_capsule.h>
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		efi-capsule {
+			image-index = <0x1>;
+			fw-version = <0x2>;
+			/* Image GUID for testing capsule update */
+			image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
+			hardware-instance = <0x0>;
+
+			blob {
+				filename = "capsule_input.bin";
+			};
+		};
+	};
+};
diff --git a/tools/binman/test/310_capsule_signed_ver.dts b/tools/binman/test/310_capsule_signed_ver.dts
new file mode 100644
index 0000000000..01c9c01222
--- /dev/null
+++ b/tools/binman/test/310_capsule_signed_ver.dts
@@ -0,0 +1,26 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+#include <sandbox_efi_capsule.h>
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		efi-capsule {
+			image-index = <0x1>;
+			fw-version = <0x2>;
+			/* Image GUID for testing capsule update */
+			image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
+			hardware-instance = <0x0>;
+			private-key = "key.key";
+			public-key-cert = "key.crt";
+
+			blob {
+				filename = "capsule_input.bin";
+			};
+		};
+	};
+};
diff --git a/tools/binman/test/311_capsule_oemflags.dts b/tools/binman/test/311_capsule_oemflags.dts
new file mode 100644
index 0000000000..cfc41c7b55
--- /dev/null
+++ b/tools/binman/test/311_capsule_oemflags.dts
@@ -0,0 +1,24 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+#include <sandbox_efi_capsule.h>
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		efi-capsule {
+			image-index = <0x1>;
+			/* Image GUID for testing capsule update */
+			image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
+			hardware-instance = <0x0>;
+			oem-flags = <0x8000>;
+
+			blob {
+				filename = "capsule_input.bin";
+			};
+		};
+	};
+};
diff --git a/tools/binman/test/312_capsule_missing_key.dts b/tools/binman/test/312_capsule_missing_key.dts
new file mode 100644
index 0000000000..1a08d0e991
--- /dev/null
+++ b/tools/binman/test/312_capsule_missing_key.dts
@@ -0,0 +1,24 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+#include <sandbox_efi_capsule.h>
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		efi-capsule {
+			image-index = <0x1>;
+			/* Image GUID for testing capsule update */
+			image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
+			hardware-instance = <0x0>;
+			private-key = "tools/binman/test/key.key";
+
+			blob {
+				filename = "capsule_input.bin";
+			};
+		};
+	};
+};
diff --git a/tools/binman/test/313_capsule_missing_index.dts b/tools/binman/test/313_capsule_missing_index.dts
new file mode 100644
index 0000000000..c6aa899211
--- /dev/null
+++ b/tools/binman/test/313_capsule_missing_index.dts
@@ -0,0 +1,22 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+#include <sandbox_efi_capsule.h>
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		efi-capsule {
+			/* Image GUID for testing capsule update */
+			image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
+			hardware-instance = <0x0>;
+
+			blob {
+				filename = "capsule_input.bin";
+			};
+		};
+	};
+};
diff --git a/tools/binman/test/314_capsule_missing_guid.dts b/tools/binman/test/314_capsule_missing_guid.dts
new file mode 100644
index 0000000000..d76afba853
--- /dev/null
+++ b/tools/binman/test/314_capsule_missing_guid.dts
@@ -0,0 +1,19 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		efi-capsule {
+			image-index = <0x1>;
+			hardware-instance = <0x0>;
+
+			blob {
+				filename = "capsule_input.bin";
+			};
+		};
+	};
+};
diff --git a/tools/binman/test/315_capsule_missing_payload.dts b/tools/binman/test/315_capsule_missing_payload.dts
new file mode 100644
index 0000000000..7d7b9e2ea0
--- /dev/null
+++ b/tools/binman/test/315_capsule_missing_payload.dts
@@ -0,0 +1,19 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+#include <sandbox_efi_capsule.h>
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		efi-capsule {
+			image-index = <0x1>;
+			/* Image GUID for testing capsule update */
+			image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
+			hardware-instance = <0x0>;
+		};
+	};
+};