| Message ID | 20230801174018.1342555-7-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Integrate EFI capsule tasks into u-boot's build flow | expand |
Hi Sughosh, On Tue, 1 Aug 2023 at 11:41, 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 V5: > * Add support for the oemflag parameter used in FWU A/B updates. This > was missed in the earlier version. > * Use a single function, generate_capsule in the mkeficapsule bintool, > instead of the multiple functions in earlier version. > * Remove the logic for generating capsules from config file as > suggested by Simon. > * Use required_props for image index and GUID parameters. > * Use a subnode for the capsule payload instead of using a filename > for the payload, as suggested by Simon. > * Add a capsule generation test with oemflag parameter being passed. > > > configs/sandbox_spl_defconfig | 1 + move to a later commit > tools/binman/btool/mkeficapsule.py | 101 +++++++++++ nit: Please split this into its own commit, with the etype in the second commit. > tools/binman/entries.rst | 64 +++++++ > tools/binman/etype/efi_capsule.py | 160 ++++++++++++++++++ > tools/binman/ftest.py | 122 +++++++++++++ > tools/binman/test/307_capsule.dts | 21 +++ > tools/binman/test/308_capsule_signed.dts | 23 +++ > tools/binman/test/309_capsule_version.dts | 22 +++ > tools/binman/test/310_capsule_signed_ver.dts | 24 +++ > tools/binman/test/311_capsule_oemflags.dts | 22 +++ > tools/binman/test/312_capsule_missing_key.dts | 22 +++ > .../binman/test/313_capsule_missing_index.dts | 20 +++ > .../binman/test/314_capsule_missing_guid.dts | 19 +++ > .../test/315_capsule_missing_payload.dts | 17 ++ > 14 files changed, 638 insertions(+) > create mode 100644 tools/binman/btool/mkeficapsule.py > 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/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig > index 8d50162b27..65223475ab 100644 > --- a/configs/sandbox_spl_defconfig > +++ b/configs/sandbox_spl_defconfig > @@ -249,3 +249,4 @@ CONFIG_UNIT_TEST=y > CONFIG_SPL_UNIT_TEST=y > CONFIG_UT_TIME=y > CONFIG_UT_DM=y > +CONFIG_TOOLS_MKEFICAPSULE=y > diff --git a/tools/binman/btool/mkeficapsule.py b/tools/binman/btool/mkeficapsule.py > new file mode 100644 > index 0000000000..da1d5de873 > --- /dev/null > +++ b/tools/binman/btool/mkeficapsule.py > @@ -0,0 +1,101 @@ > +# SPDX-License-Identifier: GPL-2.0+ > +# Copyright 2023 Linaro Limited > +# > +"""Bintool implementation for mkeficapsule tool > + > +mkeficapsule is a tool used for generating EFI capsules. > + > +The following are the command-line options to be provided > +to the tool > +Usage: mkeficapsule [options] <image blob> <output file> > +Options: > + -g, --guid <guid string> guid for image blob type > + -i, --index <index> update image index > + -I, --instance <instance> update hardware instance > + -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 > +""" > + > +from binman import bintool > + > +class Bintoolmkeficapsule(bintool.Bintool): > + """Handles the 'mkeficapsule' tool > + > + This bintool is used for generating the EFI capsules. The > + capsule generation parameters can either be specified through > + command-line, or through a config file. commandline (or command line, but please be consistent ) > + """ > + def __init__(self, name): > + super().__init__(name, 'mkeficapsule tool for generating capsules') > + > + def generate_capsule(self, image_index, image_guid, hardware_instance, > + payload, output_fname, priv_key, pub_key, > + monotonic_count=0, version=0, oemflags=0): > + """Generate a capsule through commandline provided parameters commandline-provided > + > + Args: > + image_index (int): Unique number for identifying payload image > + image_guid (str): GUID used for identifying the image > + hardware_instance (int): Optional unique hardware instance of > + a device in the system. 0 if not being used > + payload (str): Path to the input payload image > + output_fname (str): Path to the output capsule file > + priv_key (str): Path to the private key > + pub_key(str): Path to the public key > + monotonic_count (int): Count used when signing an image > + version (int): Image version (Optional) > + oemflags (int): Optional 16 bit OEM flags > + > + Returns: > + str: Tool output > + """ > + args = [ > + f'--index={image_index}', > + f'--guid={image_guid}', > + f'--instance={hardware_instance}' > + ] > + > + if version: > + args += [f'--fw-version={version}'] > + if oemflags: > + args += [f'--capoemflag={oemflags}'] > + if priv_key and pub_key: > + args += [ > + f'--monotonic-count={monotonic_count}', > + f'--private-key={priv_key}', > + f'--certificate={pub_key}' > + ] > + > + args += [ > + payload, > + output_fname > + ] > + > + return self.run_cmd(*args) > + > + def fetch(self, method): > + """Fetch handler for mkeficapsule > + > + This builds the tool from source > + > + Returns: > + tuple: > + str: Filename of fetched file to copy to a suitable directory > + str: Name of temp directory to remove, or None > + """ > + if method != bintool.FETCH_BUILD: > + return None > + > + cmd = ['tools-only_defconfig', 'tools'] > + result = self.build_from_git( > + 'https://source.denx.de/u-boot/u-boot.git', > + cmd, > + 'tools/mkeficapsule') > + return result > diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst > index f2376932be..cfe699361c 100644 > --- a/tools/binman/entries.rst > +++ b/tools/binman/entries.rst > @@ -468,6 +468,70 @@ updating the EC on startup via software sync. > > The btool looks fine to me [..] > diff --git a/tools/binman/etype/efi_capsule.py b/tools/binman/etype/efi_capsule.py > new file mode 100644 > index 0000000000..b8a269e247 > --- /dev/null > +++ b/tools/binman/etype/efi_capsule.py > @@ -0,0 +1,160 @@ > +# SPDX-License-Identifier: GPL-2.0+ > +# Copyright (c) 2023 Linaro Limited > +# > +# Entry-type module for producing a EFI capsule > +# > + > +from collections import OrderedDict > +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): > + """Entry for generating EFI capsules """Generate EPI capsules (as we know it is an entry type) > + > + This is an entry for generating EFI capsules. Drop that as it repeats the above line > + > + The parameters needed for generation of the capsules can > + either be provided as properties in the entry. or...? > + > + 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. Can you perhaps put these in a different order do you can say that if private-key is provided, you must also provide public-key-cert ? > + - pub-key-cert: Path to PEM formatted .crt public key certificate public, I think, since you write out private in full > + file. > + - capoemflags - Optional OEM flags to be passed through capsule header. oem-flags ? If 'cap' refers to capsule, we already know it is a capsule > + - filename: Technically this is not true...it is the section output. I think it is better to mention that it inherits the section property, which allows an output filename. Optional path to the output capsule file. A capsule is a > + continuous set of data as defined by the EFI specification. Refer > + to the specification for more details. Good comments. You can drop filename, since you subclass entry_Section which always provides this feature. But it's OK to mention it if you like, e.g. "All section properties are supported, include filename". > + > + 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. > + https://uefi.org/sites/default/files/resources/UEFI_Spec_2_10_Aug29.pdf I suppose this is OK, but can you make it into a text link? e.g. see how doc/usage/cmd/acpi.rst does it. > + > + 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 = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; lower-case. Also please use a name here, e.g. a #fdefine above, if this isn't something already in U-Boot. I very much want to avoid people adding random UUIDs everywhere. > + hardware-instance = <0x0>; > + private-key = "tools/binman/test/key.key"; > + pub-key-cert = "tools/binman/test/key.pem"; Drop the path for these > + capoemflags = <0x8000>; > + > + u-boot { > + }; > + }; > + > + In the above example, the capsule payload is the u-boot image. The U-Boot > + 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. > + """ > + 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.capoemflags = 0 > + self.private_key = '' > + self.pub_key_cert = '' > + self.auth = 0 > + self.capsule_fname = '' Please drop > + self._entries = OrderedDict() entry_Section already does this > + > + 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.capoemflags = fdt_util.GetInt(self._node, 'capoemflags') > + > + self.private_key = fdt_util.GetString(self._node, 'private-key') > + self.pub_key_cert = fdt_util.GetString(self._node, 'pub-key-cert') > + if ((self.private_key and not self.pub_key_cert) or (self.pub_key_cert and not self.private_key)): > + self.Raise('Both private key and public key certificate need to be provided') > + elif not (self.private_key and self.pub_key_cert): > + self.auth = 0 > + else: > + self.auth = 1 > + > + 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) != 1: > + self.Raise('The capsule entry expects a single subnode for payload') No, we mustn't restruit this. Why would we? > + > + for node in self._node.subnodes: > + entry = Entry.Create(self, node) > + entry.ReadNode() > + self._entries[entry.name] = entry > + > + def _GenCapsule(self): > + return self.mkeficapsule.generate_capsule(self.image_index, > + self.image_guid, > + self.hardware_instance, > + self.payload, > + self.capsule_fname, > + self.private_key, > + self.pub_key_cert, > + self.monotonic_count, > + self.fw_version, > + self.capoemflags) > + > + def BuildSectionData(self, required): > + if self.auth: > + if not os.path.isabs(self.private_key): > + self.private_key = tools.get_input_filename(self.private_key) > + if not os.path.isabs(self.pub_key_cert): > + self.pub_key_cert = tools.get_input_filename(self.pub_key_cert) These should be local vars, not class vars. Also these are properties passed in by the user, so you should not be converting them to filenames. > + data, self.payload, _ = self.collect_contents_to_file( Please don't add new things to the class in the middle of the class. Doesn't pylint warn about this? This can just be a local var. > + self._entries.values(), 'capsule_payload') > + outfile = self._filename if self._filename else self._node.name > + self.capsule_fname = tools.get_output_filename(outfile) No, the class is for holding properties...you don't use this outside the function, so just: fname = tools.... > + if self._GenCapsule() is not None: Here you need to pass some of the params. Honestly, why not drop _GenCapsule() and just call generate_capsule() here? It seems easier. > + os.remove(self.payload) > + return tools.read_file(self.capsule_fname) return tools.read_file(fname) > + > + def ProcessContents(self): > + ok = super().ProcessContents() > + data = self.BuildSectionData(True) > + ok2 = self.ProcessContentsUpdate(data) > + return ok and ok2 If that is the same as the entry_Section function, you can drop it > + > + def SetImagePos(self, image_pos): > + upto = 0 > + for entry in self.GetEntries().values(): > + entry.SetOffsetSize(upto, None) > + upto += entry.size > + > + super().SetImagePos(image_pos) This function looks the same as entry_Section. Drop it? > + > + def AddBintools(self, btools): > + self.mkeficapsule = self.AddBintool(btools, 'mkeficapsule') > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py > index 1cfa349d38..7e7926b607 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('efi_capsule_payload.bin', EFI_CAPSULE_DATA) > > # Add a few .dtb files for testing > TestFunctional._MakeInputFile('%s/test-fdt1.dtb' % TEST_FDT_SUBDIR, > @@ -7087,5 +7094,120 @@ 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) Can you mention where these values came from and how you created them? This all seems very brittle. Is there a tool that prints them out, or..? > + 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 a single 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..dd5b6f1c11 > --- /dev/null > +++ b/tools/binman/test/307_capsule.dts > @@ -0,0 +1,21 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +/dts-v1/; > + > +/ { > + #address-cells = <1>; > + #size-cells = <1>; > + > + binman { > + efi-capsule { > + image-index = <0x1>; > + /* Image GUID for testing capsule update */ > + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; Please create a #include files for these (and use lower case). Please fix globally. > + hardware-instance = <0x0>; > + > + blob-ext { > + filename = "efi_capsule_payload.bin"; Do you need this filename? > + }; > + }; > + }; > +}; [..] Regards, Simon
hi Simon, On Wed, 2 Aug 2023 at 18:23, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Tue, 1 Aug 2023 at 11:41, 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 V5: > > * Add support for the oemflag parameter used in FWU A/B updates. This > > was missed in the earlier version. > > * Use a single function, generate_capsule in the mkeficapsule bintool, > > instead of the multiple functions in earlier version. > > * Remove the logic for generating capsules from config file as > > suggested by Simon. > > * Use required_props for image index and GUID parameters. > > * Use a subnode for the capsule payload instead of using a filename > > for the payload, as suggested by Simon. > > * Add a capsule generation test with oemflag parameter being passed. > > > > > > configs/sandbox_spl_defconfig | 1 + > > move to a later commit Okay. I think it'd be better to have this change before adding support for efi_capsule entry in binman. > > > tools/binman/btool/mkeficapsule.py | 101 +++++++++++ > > nit: Please split this into its own commit, with the etype in the > second commit. Okay > > > tools/binman/entries.rst | 64 +++++++ > > tools/binman/etype/efi_capsule.py | 160 ++++++++++++++++++ > > tools/binman/ftest.py | 122 +++++++++++++ > > tools/binman/test/307_capsule.dts | 21 +++ > > tools/binman/test/308_capsule_signed.dts | 23 +++ > > tools/binman/test/309_capsule_version.dts | 22 +++ > > tools/binman/test/310_capsule_signed_ver.dts | 24 +++ > > tools/binman/test/311_capsule_oemflags.dts | 22 +++ > > tools/binman/test/312_capsule_missing_key.dts | 22 +++ > > .../binman/test/313_capsule_missing_index.dts | 20 +++ > > .../binman/test/314_capsule_missing_guid.dts | 19 +++ > > .../test/315_capsule_missing_payload.dts | 17 ++ > > 14 files changed, 638 insertions(+) > > create mode 100644 tools/binman/btool/mkeficapsule.py > > 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/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig > > index 8d50162b27..65223475ab 100644 > > --- a/configs/sandbox_spl_defconfig > > +++ b/configs/sandbox_spl_defconfig > > @@ -249,3 +249,4 @@ CONFIG_UNIT_TEST=y > > CONFIG_SPL_UNIT_TEST=y > > CONFIG_UT_TIME=y > > CONFIG_UT_DM=y > > +CONFIG_TOOLS_MKEFICAPSULE=y > > diff --git a/tools/binman/btool/mkeficapsule.py b/tools/binman/btool/mkeficapsule.py > > new file mode 100644 > > index 0000000000..da1d5de873 > > --- /dev/null > > +++ b/tools/binman/btool/mkeficapsule.py > > @@ -0,0 +1,101 @@ > > +# SPDX-License-Identifier: GPL-2.0+ > > +# Copyright 2023 Linaro Limited > > +# > > +"""Bintool implementation for mkeficapsule tool > > + > > +mkeficapsule is a tool used for generating EFI capsules. > > + > > +The following are the command-line options to be provided > > +to the tool > > +Usage: mkeficapsule [options] <image blob> <output file> > > +Options: > > + -g, --guid <guid string> guid for image blob type > > + -i, --index <index> update image index > > + -I, --instance <instance> update hardware instance > > + -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 > > +""" > > + > > +from binman import bintool > > + > > +class Bintoolmkeficapsule(bintool.Bintool): > > + """Handles the 'mkeficapsule' tool > > + > > + This bintool is used for generating the EFI capsules. The > > + capsule generation parameters can either be specified through > > + command-line, or through a config file. > > commandline (or command line, but please be consistent Okay > ) > > + """ > > + def __init__(self, name): > > + super().__init__(name, 'mkeficapsule tool for generating capsules') > > + > > + def generate_capsule(self, image_index, image_guid, hardware_instance, > > + payload, output_fname, priv_key, pub_key, > > + monotonic_count=0, version=0, oemflags=0): > > + """Generate a capsule through commandline provided parameters > > commandline-provided Okay > > > + > > + Args: > > + image_index (int): Unique number for identifying payload image > > + image_guid (str): GUID used for identifying the image > > + hardware_instance (int): Optional unique hardware instance of > > + a device in the system. 0 if not being used > > + payload (str): Path to the input payload image > > + output_fname (str): Path to the output capsule file > > + priv_key (str): Path to the private key > > + pub_key(str): Path to the public key > > + monotonic_count (int): Count used when signing an image > > + version (int): Image version (Optional) > > + oemflags (int): Optional 16 bit OEM flags > > + > > + Returns: > > + str: Tool output > > + """ > > + args = [ > > + f'--index={image_index}', > > + f'--guid={image_guid}', > > + f'--instance={hardware_instance}' > > + ] > > + > > + if version: > > + args += [f'--fw-version={version}'] > > + if oemflags: > > + args += [f'--capoemflag={oemflags}'] > > + if priv_key and pub_key: > > + args += [ > > + f'--monotonic-count={monotonic_count}', > > + f'--private-key={priv_key}', > > + f'--certificate={pub_key}' > > + ] > > + > > + args += [ > > + payload, > > + output_fname > > + ] > > + > > + return self.run_cmd(*args) > > + > > + def fetch(self, method): > > + """Fetch handler for mkeficapsule > > + > > + This builds the tool from source > > + > > + Returns: > > + tuple: > > + str: Filename of fetched file to copy to a suitable directory > > + str: Name of temp directory to remove, or None > > + """ > > + if method != bintool.FETCH_BUILD: > > + return None > > + > > + cmd = ['tools-only_defconfig', 'tools'] > > + result = self.build_from_git( > > + 'https://source.denx.de/u-boot/u-boot.git', > > + cmd, > > + 'tools/mkeficapsule') > > + return result > > diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst > > index f2376932be..cfe699361c 100644 > > --- a/tools/binman/entries.rst > > +++ b/tools/binman/entries.rst > > @@ -468,6 +468,70 @@ updating the EC on startup via software sync. > > > > > > The btool looks fine to me > > [..] > > > diff --git a/tools/binman/etype/efi_capsule.py b/tools/binman/etype/efi_capsule.py > > new file mode 100644 > > index 0000000000..b8a269e247 > > --- /dev/null > > +++ b/tools/binman/etype/efi_capsule.py > > @@ -0,0 +1,160 @@ > > +# SPDX-License-Identifier: GPL-2.0+ > > +# Copyright (c) 2023 Linaro Limited > > +# > > +# Entry-type module for producing a EFI capsule > > +# > > + > > +from collections import OrderedDict > > +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): > > + """Entry for generating EFI capsules > > """Generate EPI capsules > > (as we know it is an entry type) Okay > > > + > > + This is an entry for generating EFI capsules. > > Drop that as it repeats the above line Okay > > > + > > + The parameters needed for generation of the capsules can > > + either be provided as properties in the entry. > > or...? Vestige from the earlier version where capsule generation was supported through a config file as well. > > > + > > + 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. > > Can you perhaps put these in a different order do you can say that if > private-key is provided, you must also provide public-key-cert ? For both the private and public key properties, I have added the following line. "This property is mandatory for generating signed capsules." > > > + - pub-key-cert: Path to PEM formatted .crt public key certificate > > public, I think, since you write out private in full Okay > > > + file. > > + - capoemflags - Optional OEM flags to be passed through capsule header. > > oem-flags ? Okay > > If 'cap' refers to capsule, we already know it is a capsule > > > + - filename: > > Technically this is not true...it is the section output. I think it is > better to mention that it inherits the section property, which allows > an output filename. > > Optional path to the output capsule file. A capsule is a > > + continuous set of data as defined by the EFI specification. Refer > > + to the specification for more details. > > Good comments. You can drop filename, since you subclass entry_Section > which always provides this feature. But it's OK to mention it if you > like, e.g. "All section properties are supported, include filename". I have removed the filename property, and replaced it with the following text "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. > > + https://uefi.org/sites/default/files/resources/UEFI_Spec_2_10_Aug29.pdf > > I suppose this is OK, but can you make it into a text link? e.g. see > how doc/usage/cmd/acpi.rst does it. Done > > > + > > + 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 = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; > > lower-case. Also please use a name here, e.g. a #fdefine above, if > this isn't something already in U-Boot. I very much want to avoid > people adding random UUIDs everywhere. > > > + hardware-instance = <0x0>; > > + private-key = "tools/binman/test/key.key"; > > + pub-key-cert = "tools/binman/test/key.pem"; > > Drop the path for these This is just an illustration. I have removed this specific path though. > > > + capoemflags = <0x8000>; > > + > > + u-boot { > > + }; > > + }; > > + > > + In the above example, the capsule payload is the u-boot image. The > > U-Boot Okay > > > + 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. > > + """ > > + 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.capoemflags = 0 > > + self.private_key = '' > > + self.pub_key_cert = '' > > + self.auth = 0 > > + self.capsule_fname = '' > > Please drop Okay > > > + self._entries = OrderedDict() > > entry_Section already does this Removed > > > + > > + 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.capoemflags = fdt_util.GetInt(self._node, 'capoemflags') > > + > > + self.private_key = fdt_util.GetString(self._node, 'private-key') > > + self.pub_key_cert = fdt_util.GetString(self._node, 'pub-key-cert') > > + if ((self.private_key and not self.pub_key_cert) or (self.pub_key_cert and not self.private_key)): > > + self.Raise('Both private key and public key certificate need to be provided') > > + elif not (self.private_key and self.pub_key_cert): > > + self.auth = 0 > > + else: > > + self.auth = 1 > > + > > + 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) != 1: > > + self.Raise('The capsule entry expects a single subnode for payload') > > No, we mustn't restruit this. Why would we? The mkeficapsule tool that generates the capsule expects a single image as it's input payload. Why do you see the need to support multiple entries as payload? mkeficapsule [options] <image blob> <output file> > > > + > > + for node in self._node.subnodes: > > + entry = Entry.Create(self, node) > > + entry.ReadNode() > > + self._entries[entry.name] = entry > > + > > + def _GenCapsule(self): > > + return self.mkeficapsule.generate_capsule(self.image_index, > > + self.image_guid, > > + self.hardware_instance, > > + self.payload, > > + self.capsule_fname, > > + self.private_key, > > + self.pub_key_cert, > > + self.monotonic_count, > > + self.fw_version, > > + self.capoemflags) > > + > > + def BuildSectionData(self, required): > > + if self.auth: > > + if not os.path.isabs(self.private_key): > > + self.private_key = tools.get_input_filename(self.private_key) > > + if not os.path.isabs(self.pub_key_cert): > > + self.pub_key_cert = tools.get_input_filename(self.pub_key_cert) > > These should be local vars, not class vars. ? These are properties being passed by the user. Why should only these be local vars? > Also these are properties > passed in by the user, so you should not be converting them to > filenames. These properties are actually paths to the private and public key cert files that a user will pass. In the normal usage, the user would be expected to provide the absolute path to the private and public key cert files. The above logic is needed for binman tests and out of tree builds with relative paths. > > > + data, self.payload, _ = self.collect_contents_to_file( > > Please don't add new things to the class in the middle of the class. > Doesn't pylint warn about this? This can just be a local var. Changed this to a local var. > > + self._entries.values(), 'capsule_payload') > > + outfile = self._filename if self._filename else self._node.name > > + self.capsule_fname = tools.get_output_filename(outfile) > > No, the class is for holding properties...you don't use this outside > the function, so just: > > fname = tools.... Changed > > > + if self._GenCapsule() is not None: > > Here you need to pass some of the params. Honestly, why not drop > _GenCapsule() and just call generate_capsule() here? It seems easier. Done > > > + os.remove(self.payload) > > + return tools.read_file(self.capsule_fname) > > return tools.read_file(fname) > > > + > > + def ProcessContents(self): > > + ok = super().ProcessContents() > > + data = self.BuildSectionData(True) > > + ok2 = self.ProcessContentsUpdate(data) > > + return ok and ok2 > > If that is the same as the entry_Section function, you can drop it There are differences, so this is needed. > > > + > > + def SetImagePos(self, image_pos): > > + upto = 0 > > + for entry in self.GetEntries().values(): > > + entry.SetOffsetSize(upto, None) > > + upto += entry.size > > + > > + super().SetImagePos(image_pos) > > This function looks the same as entry_Section. Drop it? Yes > > > + > > + def AddBintools(self, btools): > > + self.mkeficapsule = self.AddBintool(btools, 'mkeficapsule') > > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py > > index 1cfa349d38..7e7926b607 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('efi_capsule_payload.bin', EFI_CAPSULE_DATA) > > > > # Add a few .dtb files for testing > > TestFunctional._MakeInputFile('%s/test-fdt1.dtb' % TEST_FDT_SUBDIR, > > @@ -7087,5 +7094,120 @@ 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) > > Can you mention where these values came from and how you created them? > This all seems very brittle. Is there a tool that prints them out, > or..? Like I mentioned earlier as well, the contents of a capsule are dependent on the input parameters specified in it's generation. For e.g. the capsule generated would be different when opting for passing the version param. Similarly when generating a signed capsule -- even with a signed capsule, the contents would depend on the keys. The offsets therefore are not fixed but depend on the inputs specified. I have added comments here for better understanding. Not sure what can be done to improve this. This is also one reason why the testing of the EFI capsule update feature should use capsules generated as part of the sandbox platform build. That makes the testing that much more robust. > > > + 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 a single 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..dd5b6f1c11 > > --- /dev/null > > +++ b/tools/binman/test/307_capsule.dts > > @@ -0,0 +1,21 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > + > > +/dts-v1/; > > + > > +/ { > > + #address-cells = <1>; > > + #size-cells = <1>; > > + > > + binman { > > + efi-capsule { > > + image-index = <0x1>; > > + /* Image GUID for testing capsule update */ > > + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; > > Please create a #include files for these (and use lower case). Please > fix globally. Okay. I am adding a file sandbox_efi_capsule.h which contains information related to the capsule GUIDs and input files needed for EFI capsule update functionality. > > > + hardware-instance = <0x0>; > > + > > + blob-ext { > > + filename = "efi_capsule_payload.bin"; > > Do you need this filename? Yes, this is the payload that will be used in generating the capsule. -sughosh > > > + }; > > + }; > > + }; > > +}; > > [..] > > Regards, > Simon
Hi Sughosh, On Thu, 3 Aug 2023 at 05:08, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > hi Simon, > > On Wed, 2 Aug 2023 at 18:23, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Tue, 1 Aug 2023 at 11:41, 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 V5: > > > * Add support for the oemflag parameter used in FWU A/B updates. This > > > was missed in the earlier version. > > > * Use a single function, generate_capsule in the mkeficapsule bintool, > > > instead of the multiple functions in earlier version. > > > * Remove the logic for generating capsules from config file as > > > suggested by Simon. > > > * Use required_props for image index and GUID parameters. > > > * Use a subnode for the capsule payload instead of using a filename > > > for the payload, as suggested by Simon. > > > * Add a capsule generation test with oemflag parameter being passed. > > > > > > > > > configs/sandbox_spl_defconfig | 1 + > > > > move to a later commit > > Okay. I think it'd be better to have this change before adding support > for efi_capsule entry in binman. OK, I see that it is currently only built for tools-only and a smattering of other boards. Tools should be built for all boards. I suppose for now you can create a patch to enable it for all sandbox boards, e.g. 'default y if SANDBOX' in the Kconfing, in a patch before this one. > > > > > > > tools/binman/btool/mkeficapsule.py | 101 +++++++++++ > > > > nit: Please split this into its own commit, with the etype in the > > second commit. > > Okay > > > > > > tools/binman/entries.rst | 64 +++++++ > > > tools/binman/etype/efi_capsule.py | 160 ++++++++++++++++++ > > > tools/binman/ftest.py | 122 +++++++++++++ > > > tools/binman/test/307_capsule.dts | 21 +++ > > > tools/binman/test/308_capsule_signed.dts | 23 +++ > > > tools/binman/test/309_capsule_version.dts | 22 +++ > > > tools/binman/test/310_capsule_signed_ver.dts | 24 +++ > > > tools/binman/test/311_capsule_oemflags.dts | 22 +++ > > > tools/binman/test/312_capsule_missing_key.dts | 22 +++ > > > .../binman/test/313_capsule_missing_index.dts | 20 +++ > > > .../binman/test/314_capsule_missing_guid.dts | 19 +++ > > > .../test/315_capsule_missing_payload.dts | 17 ++ > > > 14 files changed, 638 insertions(+) > > > create mode 100644 tools/binman/btool/mkeficapsule.py > > > 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/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig > > > index 8d50162b27..65223475ab 100644 > > > --- a/configs/sandbox_spl_defconfig > > > +++ b/configs/sandbox_spl_defconfig > > > @@ -249,3 +249,4 @@ CONFIG_UNIT_TEST=y > > > CONFIG_SPL_UNIT_TEST=y > > > CONFIG_UT_TIME=y > > > CONFIG_UT_DM=y > > > +CONFIG_TOOLS_MKEFICAPSULE=y [..] > > > + 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) != 1: > > > + self.Raise('The capsule entry expects a single subnode for payload') > > > > No, we mustn't restruit this. Why would we? > > The mkeficapsule tool that generates the capsule expects a single > image as it's input payload. Why do you see the need to support > multiple entries as payload? That's how binman works...it isn't the entry type's job to decide how the image is laid out. The user could add a section in any cases and get around any restriction you put here. But the restriction isn't necessary and is just strange. > > mkeficapsule [options] <image blob> <output file> > > > > > > > + > > > + for node in self._node.subnodes: > > > + entry = Entry.Create(self, node) > > > + entry.ReadNode() > > > + self._entries[entry.name] = entry > > > + > > > + def _GenCapsule(self): > > > + return self.mkeficapsule.generate_capsule(self.image_index, > > > + self.image_guid, > > > + self.hardware_instance, > > > + self.payload, > > > + self.capsule_fname, > > > + self.private_key, > > > + self.pub_key_cert, > > > + self.monotonic_count, > > > + self.fw_version, > > > + self.capoemflags) > > > + > > > + def BuildSectionData(self, required): > > > + if self.auth: > > > + if not os.path.isabs(self.private_key): > > > + self.private_key = tools.get_input_filename(self.private_key) > > > + if not os.path.isabs(self.pub_key_cert): > > > + self.pub_key_cert = tools.get_input_filename(self.pub_key_cert) > > > > These should be local vars, not class vars. > > ? > > These are properties being passed by the user. Why should only these > be local vars? Not really. self.private_key is a string, but here you turn it into an input filename. You are changing the original property. Just use a local var. > > > Also these are properties > > passed in by the user, so you should not be converting them to > > filenames. > > These properties are actually paths to the private and public key cert > files that a user will pass. In the normal usage, the user would be > expected to provide the absolute path to the private and public key > cert files. The above logic is needed for binman tests and out of tree > builds with relative paths. I'm fine with the logic, just don't update the property. > > > > > > + data, self.payload, _ = self.collect_contents_to_file( > > > > Please don't add new things to the class in the middle of the class. > > Doesn't pylint warn about this? This can just be a local var. > > Changed this to a local var. > > > > + self._entries.values(), 'capsule_payload') > > > + outfile = self._filename if self._filename else self._node.name > > > + self.capsule_fname = tools.get_output_filename(outfile) > > > > No, the class is for holding properties...you don't use this outside > > the function, so just: [..] > > > + def ProcessContents(self): > > > + ok = super().ProcessContents() > > > + data = self.BuildSectionData(True) > > > + ok2 = self.ProcessContentsUpdate(data) > > > + return ok and ok2 > > > > If that is the same as the entry_Section function, you can drop it > > There are differences, so this is needed. Why is it different? Deleting the code has no effect on the tests that I can see. [..] > > > + 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) > > > > Can you mention where these values came from and how you created them? > > This all seems very brittle. Is there a tool that prints them out, > > or..? > > Like I mentioned earlier as well, the contents of a capsule are > dependent on the input parameters specified in it's generation. For > e.g. the capsule generated would be different when opting for passing > the version param. Similarly when generating a signed capsule -- even > with a signed capsule, the contents would depend on the keys. The > offsets therefore are not fixed but depend on the inputs specified. I > have added comments here for better understanding. Not sure what can > be done to improve this. > > This is also one reason why the testing of the EFI capsule update > feature should use capsules generated as part of the sandbox platform > build. That makes the testing that much more robust. We have the same problem with mkimage. Is there a 'dump_capsule' command that can output information about it? Then you could use that in this test. > > > > > > + 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) [..] > > > > > > + hardware-instance = <0x0>; > > > + > > > + blob-ext { > > > + filename = "efi_capsule_payload.bin"; > > > > Do you need this filename? > > Yes, this is the payload that will be used in generating the capsule. Oh, this is so confusing. I think you can just make this a 'blob' instead of a 'blob-ext'. How about capsule_input.bin as it is shorter? [..] Regards, Simon
hi Simon, On Fri, 4 Aug 2023 at 08:32, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Thu, 3 Aug 2023 at 05:08, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > hi Simon, > > > > On Wed, 2 Aug 2023 at 18:23, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Tue, 1 Aug 2023 at 11:41, 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 V5: > > > > * Add support for the oemflag parameter used in FWU A/B updates. This > > > > was missed in the earlier version. > > > > * Use a single function, generate_capsule in the mkeficapsule bintool, > > > > instead of the multiple functions in earlier version. > > > > * Remove the logic for generating capsules from config file as > > > > suggested by Simon. > > > > * Use required_props for image index and GUID parameters. > > > > * Use a subnode for the capsule payload instead of using a filename > > > > for the payload, as suggested by Simon. > > > > * Add a capsule generation test with oemflag parameter being passed. > > > > > > > > > > > > configs/sandbox_spl_defconfig | 1 + > > > > > > move to a later commit > > > > Okay. I think it'd be better to have this change before adding support > > for efi_capsule entry in binman. > > OK, I see that it is currently only built for tools-only and a > smattering of other boards. Tools should be built for all boards. I > suppose for now you can create a patch to enable it for all sandbox > boards, e.g. 'default y if SANDBOX' in the Kconfing, in a patch before > this one. Will do. > > > > > > > > > > > > tools/binman/btool/mkeficapsule.py | 101 +++++++++++ > > > > > > nit: Please split this into its own commit, with the etype in the > > > second commit. > > > > Okay > > > > > > > > > tools/binman/entries.rst | 64 +++++++ > > > > tools/binman/etype/efi_capsule.py | 160 ++++++++++++++++++ > > > > tools/binman/ftest.py | 122 +++++++++++++ > > > > tools/binman/test/307_capsule.dts | 21 +++ > > > > tools/binman/test/308_capsule_signed.dts | 23 +++ > > > > tools/binman/test/309_capsule_version.dts | 22 +++ > > > > tools/binman/test/310_capsule_signed_ver.dts | 24 +++ > > > > tools/binman/test/311_capsule_oemflags.dts | 22 +++ > > > > tools/binman/test/312_capsule_missing_key.dts | 22 +++ > > > > .../binman/test/313_capsule_missing_index.dts | 20 +++ > > > > .../binman/test/314_capsule_missing_guid.dts | 19 +++ > > > > .../test/315_capsule_missing_payload.dts | 17 ++ > > > > 14 files changed, 638 insertions(+) > > > > create mode 100644 tools/binman/btool/mkeficapsule.py > > > > 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/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig > > > > index 8d50162b27..65223475ab 100644 > > > > --- a/configs/sandbox_spl_defconfig > > > > +++ b/configs/sandbox_spl_defconfig > > > > @@ -249,3 +249,4 @@ CONFIG_UNIT_TEST=y > > > > CONFIG_SPL_UNIT_TEST=y > > > > CONFIG_UT_TIME=y > > > > CONFIG_UT_DM=y > > > > +CONFIG_TOOLS_MKEFICAPSULE=y > > [..] > > > > > + 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) != 1: > > > > + self.Raise('The capsule entry expects a single subnode for payload') > > > > > > No, we mustn't restruit this. Why would we? > > > > The mkeficapsule tool that generates the capsule expects a single > > image as it's input payload. Why do you see the need to support > > multiple entries as payload? > > That's how binman works...it isn't the entry type's job to decide how > the image is laid out. The user could add a section in any cases and > get around any restriction you put here. But the restriction isn't > necessary and is just strange. The idea here is that if a user puts two entries, there is no clarity on how that is to be handled, since the mkeficapsule tool expects a single input image(binary in binman jargon). I am aware of the user using section type to club multiple images, and I think that is absolutely fine, since we are then left with one final binary to present to the mkeficapsule tool. In fact, I think if the user wants a combination of multiple binaries, they can use a section entry. In fact a user can even use the recently added templates in binman to add stuff to the input binary, but the final binary to be passed to the mkeficapsule tool should be one. > > > > > mkeficapsule [options] <image blob> <output file> > > > > > > > > > > > + > > > > + for node in self._node.subnodes: > > > > + entry = Entry.Create(self, node) > > > > + entry.ReadNode() > > > > + self._entries[entry.name] = entry > > > > + > > > > + def _GenCapsule(self): > > > > + return self.mkeficapsule.generate_capsule(self.image_index, > > > > + self.image_guid, > > > > + self.hardware_instance, > > > > + self.payload, > > > > + self.capsule_fname, > > > > + self.private_key, > > > > + self.pub_key_cert, > > > > + self.monotonic_count, > > > > + self.fw_version, > > > > + self.capoemflags) > > > > + > > > > + def BuildSectionData(self, required): > > > > + if self.auth: > > > > + if not os.path.isabs(self.private_key): > > > > + self.private_key = tools.get_input_filename(self.private_key) > > > > + if not os.path.isabs(self.pub_key_cert): > > > > + self.pub_key_cert = tools.get_input_filename(self.pub_key_cert) > > > > > > These should be local vars, not class vars. > > > > ? > > > > These are properties being passed by the user. Why should only these > > be local vars? > > Not really. > > self.private_key is a string, but here you turn it into an input > filename. You are changing the original property. Just use a local > var. Okay, understood. > > > > > > Also these are properties > > > passed in by the user, so you should not be converting them to > > > filenames. > > > > These properties are actually paths to the private and public key cert > > files that a user will pass. In the normal usage, the user would be > > expected to provide the absolute path to the private and public key > > cert files. The above logic is needed for binman tests and out of tree > > builds with relative paths. > > I'm fine with the logic, just don't update the property. Understood > > > > > > > > > > + data, self.payload, _ = self.collect_contents_to_file( > > > > > > Please don't add new things to the class in the middle of the class. > > > Doesn't pylint warn about this? This can just be a local var. > > > > Changed this to a local var. > > > > > > + self._entries.values(), 'capsule_payload') > > > > + outfile = self._filename if self._filename else self._node.name > > > > + self.capsule_fname = tools.get_output_filename(outfile) > > > > > > No, the class is for holding properties...you don't use this outside > > > the function, so just: > > [..] > > > > > + def ProcessContents(self): > > > > + ok = super().ProcessContents() > > > > + data = self.BuildSectionData(True) > > > > + ok2 = self.ProcessContentsUpdate(data) > > > > + return ok and ok2 > > > > > > If that is the same as the entry_Section function, you can drop it > > > > There are differences, so this is needed. > > Why is it different? Deleting the code has no effect on the tests that > I can see. Umm, let me check this, but a cursory glance showed differences. This is in fact taken from the mkimage.py entry. > > [..] > > > > > + 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) > > > > > > Can you mention where these values came from and how you created them? > > > This all seems very brittle. Is there a tool that prints them out, > > > or..? > > > > Like I mentioned earlier as well, the contents of a capsule are > > dependent on the input parameters specified in it's generation. For > > e.g. the capsule generated would be different when opting for passing > > the version param. Similarly when generating a signed capsule -- even > > with a signed capsule, the contents would depend on the keys. The > > offsets therefore are not fixed but depend on the inputs specified. I > > have added comments here for better understanding. Not sure what can > > be done to improve this. > > > > This is also one reason why the testing of the EFI capsule update > > feature should use capsules generated as part of the sandbox platform > > build. That makes the testing that much more robust. > > We have the same problem with mkimage. Is there a 'dump_capsule' > command that can output information about it? Then you could use that > in this test. We currently do not have support in the mkeficapsule tool to dump the capsule contents. I can take a stab at it later. I would say, for now, let's get this in. Once I have made changes to the capsule generation tool, I will come back and change it. > > > > > > > > > > + 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) > > [..] > > > > > > > > > > + hardware-instance = <0x0>; > > > > + > > > > + blob-ext { > > > > + filename = "efi_capsule_payload.bin"; > > > > > > Do you need this filename? > > > > Yes, this is the payload that will be used in generating the capsule. > > Oh, this is so confusing. I think you can just make this a 'blob' > instead of a 'blob-ext'. How about capsule_input.bin as it is shorter? Heh, not sure what is confusing. This is a blob which is specifying the name of the image(binary) which goes into the capsule. If you think the name is rather long, I will make it shorter as per your suggestion. -sughosh
Hi Sughosh, On Fri, 4 Aug 2023 at 00:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > hi Simon, > > On Fri, 4 Aug 2023 at 08:32, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Thu, 3 Aug 2023 at 05:08, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > hi Simon, > > > > > > On Wed, 2 Aug 2023 at 18:23, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Sughosh, > > > > > > > > On Tue, 1 Aug 2023 at 11:41, 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 V5: > > > > > * Add support for the oemflag parameter used in FWU A/B updates. This > > > > > was missed in the earlier version. > > > > > * Use a single function, generate_capsule in the mkeficapsule bintool, > > > > > instead of the multiple functions in earlier version. > > > > > * Remove the logic for generating capsules from config file as > > > > > suggested by Simon. > > > > > * Use required_props for image index and GUID parameters. > > > > > * Use a subnode for the capsule payload instead of using a filename > > > > > for the payload, as suggested by Simon. > > > > > * Add a capsule generation test with oemflag parameter being passed. > > > > > > > > > > > > > > > configs/sandbox_spl_defconfig | 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) != 1: > > > > > + self.Raise('The capsule entry expects a single subnode for payload') > > > > > > > > No, we mustn't restruit this. Why would we? > > > > > > The mkeficapsule tool that generates the capsule expects a single > > > image as it's input payload. Why do you see the need to support > > > multiple entries as payload? > > > > That's how binman works...it isn't the entry type's job to decide how > > the image is laid out. The user could add a section in any cases and > > get around any restriction you put here. But the restriction isn't > > necessary and is just strange. > > The idea here is that if a user puts two entries, there is no clarity > on how that is to be handled, since the mkeficapsule tool expects a > single input image(binary in binman jargon). I am aware of the user There is a single input, since binman collects everything together. > using section type to club multiple images, and I think that is > absolutely fine, since we are then left with one final binary to > present to the mkeficapsule tool. In fact, I think if the user wants a > combination of multiple binaries, they can use a section entry. In > fact a user can even use the recently added templates in binman to add > stuff to the input binary, but the final binary to be passed to the > mkeficapsule tool should be one. Is there some strange magic in the tool? What does it matter where the data come from? I really don't understand what breaks if the user adds two entries here. We should not have this limitation. [..] > > > > [..] > > > > > > > + 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) > > > > > > > > Can you mention where these values came from and how you created them? > > > > This all seems very brittle. Is there a tool that prints them out, > > > > or..? > > > > > > Like I mentioned earlier as well, the contents of a capsule are > > > dependent on the input parameters specified in it's generation. For > > > e.g. the capsule generated would be different when opting for passing > > > the version param. Similarly when generating a signed capsule -- even > > > with a signed capsule, the contents would depend on the keys. The > > > offsets therefore are not fixed but depend on the inputs specified. I > > > have added comments here for better understanding. Not sure what can > > > be done to improve this. > > > > > > This is also one reason why the testing of the EFI capsule update > > > feature should use capsules generated as part of the sandbox platform > > > build. That makes the testing that much more robust. > > > > We have the same problem with mkimage. Is there a 'dump_capsule' > > command that can output information about it? Then you could use that > > in this test. > > We currently do not have support in the mkeficapsule tool to dump the > capsule contents. I can take a stab at it later. I would say, for now, > let's get this in. Once I have made changes to the capsule generation > tool, I will come back and change it. OK, I think that is a good path. > > > > > > > > > > > > > > > + 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) > > > > [..] > > > > > > > > > > > > > > + hardware-instance = <0x0>; > > > > > + > > > > > + blob-ext { > > > > > + filename = "efi_capsule_payload.bin"; > > > > > > > > Do you need this filename? > > > > > > Yes, this is the payload that will be used in generating the capsule. > > > > Oh, this is so confusing. I think you can just make this a 'blob' > > instead of a 'blob-ext'. How about capsule_input.bin as it is shorter? > > Heh, not sure what is confusing. This is a blob which is specifying > the name of the image(binary) which goes into the capsule. If you > think the name is rather long, I will make it shorter as per your > suggestion. Well making it blob-ext was suggesting there was some external thing that might not be available. If you change it to 'blob' it is easier. Yes 'input' is better as 'payload' sounds like it is the capsule update file. Regards, Smion
diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig index 8d50162b27..65223475ab 100644 --- a/configs/sandbox_spl_defconfig +++ b/configs/sandbox_spl_defconfig @@ -249,3 +249,4 @@ CONFIG_UNIT_TEST=y CONFIG_SPL_UNIT_TEST=y CONFIG_UT_TIME=y CONFIG_UT_DM=y +CONFIG_TOOLS_MKEFICAPSULE=y diff --git a/tools/binman/btool/mkeficapsule.py b/tools/binman/btool/mkeficapsule.py new file mode 100644 index 0000000000..da1d5de873 --- /dev/null +++ b/tools/binman/btool/mkeficapsule.py @@ -0,0 +1,101 @@ +# SPDX-License-Identifier: GPL-2.0+ +# Copyright 2023 Linaro Limited +# +"""Bintool implementation for mkeficapsule tool + +mkeficapsule is a tool used for generating EFI capsules. + +The following are the command-line options to be provided +to the tool +Usage: mkeficapsule [options] <image blob> <output file> +Options: + -g, --guid <guid string> guid for image blob type + -i, --index <index> update image index + -I, --instance <instance> update hardware instance + -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 +""" + +from binman import bintool + +class Bintoolmkeficapsule(bintool.Bintool): + """Handles the 'mkeficapsule' tool + + This bintool is used for generating the EFI capsules. The + capsule generation parameters can either be specified through + command-line, or through a config file. + """ + def __init__(self, name): + super().__init__(name, 'mkeficapsule tool for generating capsules') + + def generate_capsule(self, image_index, image_guid, hardware_instance, + payload, output_fname, priv_key, pub_key, + monotonic_count=0, version=0, oemflags=0): + """Generate a capsule through commandline provided parameters + + Args: + image_index (int): Unique number for identifying payload image + image_guid (str): GUID used for identifying the image + hardware_instance (int): Optional unique hardware instance of + a device in the system. 0 if not being used + payload (str): Path to the input payload image + output_fname (str): Path to the output capsule file + priv_key (str): Path to the private key + pub_key(str): Path to the public key + monotonic_count (int): Count used when signing an image + version (int): Image version (Optional) + oemflags (int): Optional 16 bit OEM flags + + Returns: + str: Tool output + """ + args = [ + f'--index={image_index}', + f'--guid={image_guid}', + f'--instance={hardware_instance}' + ] + + if version: + args += [f'--fw-version={version}'] + if oemflags: + args += [f'--capoemflag={oemflags}'] + if priv_key and pub_key: + args += [ + f'--monotonic-count={monotonic_count}', + f'--private-key={priv_key}', + f'--certificate={pub_key}' + ] + + args += [ + payload, + output_fname + ] + + return self.run_cmd(*args) + + def fetch(self, method): + """Fetch handler for mkeficapsule + + This builds the tool from source + + Returns: + tuple: + str: Filename of fetched file to copy to a suitable directory + str: Name of temp directory to remove, or None + """ + if method != bintool.FETCH_BUILD: + return None + + cmd = ['tools-only_defconfig', 'tools'] + result = self.build_from_git( + 'https://source.denx.de/u-boot/u-boot.git', + cmd, + 'tools/mkeficapsule') + return result diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst index f2376932be..cfe699361c 100644 --- a/tools/binman/entries.rst +++ b/tools/binman/entries.rst @@ -468,6 +468,70 @@ updating the EC on startup via software sync. +.. _etype_efi_capsule: + +Entry: capsule: Entry for generating EFI Capsule files +------------------------------------------------------ + +This is an entry for generating EFI capsules. + + This is an entry for generating EFI capsules. + + The parameters needed for generation of the capsules can + either be provided 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. + - pub-key-cert: Path to PEM formatted .crt public key certificate + file. + - capoemflags - Optional OEM flags to be passed through capsule header. + - filename: Optional path to the output capsule file. A capsule is a + continuous set of data as defined by the EFI specification. Refer + to the specification for more details. + + 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. + https://uefi.org/sites/default/files/resources/UEFI_Spec_2_10_Aug29.pdf + + 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 = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; + hardware-instance = <0x0>; + private-key = "tools/binman/test/key.key"; + pub-key-cert = "tools/binman/test/key.pem"; + filename = "test.capsule"; + + 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. + + + .. _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..b8a269e247 --- /dev/null +++ b/tools/binman/etype/efi_capsule.py @@ -0,0 +1,160 @@ +# SPDX-License-Identifier: GPL-2.0+ +# Copyright (c) 2023 Linaro Limited +# +# Entry-type module for producing a EFI capsule +# + +from collections import OrderedDict +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): + """Entry for generating EFI capsules + + This is an entry for generating EFI capsules. + + The parameters needed for generation of the capsules can + either be provided 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. + - pub-key-cert: Path to PEM formatted .crt public key certificate + file. + - capoemflags - Optional OEM flags to be passed through capsule header. + - filename: Optional path to the output capsule file. A capsule is a + continuous set of data as defined by the EFI specification. Refer + to the specification for more details. + + 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. + https://uefi.org/sites/default/files/resources/UEFI_Spec_2_10_Aug29.pdf + + 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 = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; + hardware-instance = <0x0>; + private-key = "tools/binman/test/key.key"; + pub-key-cert = "tools/binman/test/key.pem"; + capoemflags = <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. + """ + 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.capoemflags = 0 + self.private_key = '' + self.pub_key_cert = '' + self.auth = 0 + self.capsule_fname = '' + self._entries = OrderedDict() + + 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.capoemflags = fdt_util.GetInt(self._node, 'capoemflags') + + self.private_key = fdt_util.GetString(self._node, 'private-key') + self.pub_key_cert = fdt_util.GetString(self._node, 'pub-key-cert') + if ((self.private_key and not self.pub_key_cert) or (self.pub_key_cert and not self.private_key)): + self.Raise('Both private key and public key certificate need to be provided') + elif not (self.private_key and self.pub_key_cert): + self.auth = 0 + else: + self.auth = 1 + + 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) != 1: + self.Raise('The capsule entry expects a single subnode for payload') + + for node in self._node.subnodes: + entry = Entry.Create(self, node) + entry.ReadNode() + self._entries[entry.name] = entry + + def _GenCapsule(self): + return self.mkeficapsule.generate_capsule(self.image_index, + self.image_guid, + self.hardware_instance, + self.payload, + self.capsule_fname, + self.private_key, + self.pub_key_cert, + self.monotonic_count, + self.fw_version, + self.capoemflags) + + def BuildSectionData(self, required): + if self.auth: + if not os.path.isabs(self.private_key): + self.private_key = tools.get_input_filename(self.private_key) + if not os.path.isabs(self.pub_key_cert): + self.pub_key_cert = tools.get_input_filename(self.pub_key_cert) + data, self.payload, _ = self.collect_contents_to_file( + self._entries.values(), 'capsule_payload') + outfile = self._filename if self._filename else self._node.name + self.capsule_fname = tools.get_output_filename(outfile) + if self._GenCapsule() is not None: + os.remove(self.payload) + return tools.read_file(self.capsule_fname) + + def ProcessContents(self): + ok = super().ProcessContents() + data = self.BuildSectionData(True) + ok2 = self.ProcessContentsUpdate(data) + return ok and ok2 + + def SetImagePos(self, image_pos): + upto = 0 + for entry in self.GetEntries().values(): + entry.SetOffsetSize(upto, None) + upto += entry.size + + super().SetImagePos(image_pos) + + def AddBintools(self, btools): + self.mkeficapsule = self.AddBintool(btools, 'mkeficapsule') diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 1cfa349d38..7e7926b607 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('efi_capsule_payload.bin', EFI_CAPSULE_DATA) # Add a few .dtb files for testing TestFunctional._MakeInputFile('%s/test-fdt1.dtb' % TEST_FDT_SUBDIR, @@ -7087,5 +7094,120 @@ 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 a single 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..dd5b6f1c11 --- /dev/null +++ b/tools/binman/test/307_capsule.dts @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + efi-capsule { + image-index = <0x1>; + /* Image GUID for testing capsule update */ + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; + hardware-instance = <0x0>; + + blob-ext { + filename = "efi_capsule_payload.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..2765dd4140 --- /dev/null +++ b/tools/binman/test/308_capsule_signed.dts @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + efi-capsule { + image-index = <0x1>; + /* Image GUID for testing capsule update */ + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; + hardware-instance = <0x0>; + private-key = "key.key"; + pub-key-cert = "key.crt"; + + blob-ext { + filename = "efi_capsule_payload.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..21d6e2bd26 --- /dev/null +++ b/tools/binman/test/309_capsule_version.dts @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + efi-capsule { + image-index = <0x1>; + fw-version = <0x2>; + /* Image GUID for testing capsule update */ + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; + hardware-instance = <0x0>; + + blob-ext { + filename = "efi_capsule_payload.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..f3606e6520 --- /dev/null +++ b/tools/binman/test/310_capsule_signed_ver.dts @@ -0,0 +1,24 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + efi-capsule { + image-index = <0x1>; + fw-version = <0x2>; + /* Image GUID for testing capsule update */ + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; + hardware-instance = <0x0>; + private-key = "key.key"; + pub-key-cert = "key.crt"; + + blob-ext { + filename = "efi_capsule_payload.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..9f535f8712 --- /dev/null +++ b/tools/binman/test/311_capsule_oemflags.dts @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + efi-capsule { + image-index = <0x1>; + /* Image GUID for testing capsule update */ + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; + hardware-instance = <0x0>; + capoemflags = <0x8000>; + + blob-ext { + filename = "efi_capsule_payload.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..6a6fc59b6d --- /dev/null +++ b/tools/binman/test/312_capsule_missing_key.dts @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + efi-capsule { + image-index = <0x1>; + /* Image GUID for testing capsule update */ + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; + hardware-instance = <0x0>; + private-key = "tools/binman/test/key.key"; + + blob-ext { + filename = "efi_capsule_payload.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..7806521135 --- /dev/null +++ b/tools/binman/test/313_capsule_missing_index.dts @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + efi-capsule { + /* Image GUID for testing capsule update */ + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; + hardware-instance = <0x0>; + + blob-ext { + filename = "efi_capsule_payload.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..599562bef6 --- /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-ext { + filename = "efi_capsule_payload.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..86da9cd309 --- /dev/null +++ b/tools/binman/test/315_capsule_missing_payload.dts @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + efi-capsule { + image-index = <0x1>; + /* Image GUID for testing capsule update */ + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; + hardware-instance = <0x0>; + }; + }; +};
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 V5: * Add support for the oemflag parameter used in FWU A/B updates. This was missed in the earlier version. * Use a single function, generate_capsule in the mkeficapsule bintool, instead of the multiple functions in earlier version. * Remove the logic for generating capsules from config file as suggested by Simon. * Use required_props for image index and GUID parameters. * Use a subnode for the capsule payload instead of using a filename for the payload, as suggested by Simon. * Add a capsule generation test with oemflag parameter being passed. configs/sandbox_spl_defconfig | 1 + tools/binman/btool/mkeficapsule.py | 101 +++++++++++ tools/binman/entries.rst | 64 +++++++ tools/binman/etype/efi_capsule.py | 160 ++++++++++++++++++ tools/binman/ftest.py | 122 +++++++++++++ tools/binman/test/307_capsule.dts | 21 +++ tools/binman/test/308_capsule_signed.dts | 23 +++ tools/binman/test/309_capsule_version.dts | 22 +++ tools/binman/test/310_capsule_signed_ver.dts | 24 +++ tools/binman/test/311_capsule_oemflags.dts | 22 +++ tools/binman/test/312_capsule_missing_key.dts | 22 +++ .../binman/test/313_capsule_missing_index.dts | 20 +++ .../binman/test/314_capsule_missing_guid.dts | 19 +++ .../test/315_capsule_missing_payload.dts | 17 ++ 14 files changed, 638 insertions(+) create mode 100644 tools/binman/btool/mkeficapsule.py 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