Hi Sugosh, On Wed, 4 Oct 2023 at 05:27, Sughosh Ganu <sughosh.g...@linaro.org> wrote: > > Add support in binman for generating EFI empty capsules. These > capsules are used in the FWU A/B update feature. Also add test cases > in binman for the corresponding code coverage. > > Signed-off-by: Sughosh Ganu <sughosh.g...@linaro.org> > --- > tools/binman/etype/efi_empty_capsule.py | 91 +++++++++++++++++++ > tools/binman/ftest.py | 52 +++++++++++ > tools/binman/test/319_capsule_accept.dts | 16 ++++ > tools/binman/test/320_capsule_revert.dts | 14 +++ > .../test/321_capsule_accept_missing_guid.dts | 14 +++ > .../binman/test/322_capsule_accept_revert.dts | 17 ++++ > 6 files changed, 204 insertions(+) > create mode 100644 tools/binman/etype/efi_empty_capsule.py > create mode 100644 tools/binman/test/319_capsule_accept.dts > create mode 100644 tools/binman/test/320_capsule_revert.dts > create mode 100644 tools/binman/test/321_capsule_accept_missing_guid.dts > create mode 100644 tools/binman/test/322_capsule_accept_revert.dts > > diff --git a/tools/binman/etype/efi_empty_capsule.py > b/tools/binman/etype/efi_empty_capsule.py > new file mode 100644 > index 0000000000..d2c781627b > --- /dev/null > +++ b/tools/binman/etype/efi_empty_capsule.py > @@ -0,0 +1,91 @@ > +# SPDX-License-Identifier: GPL-2.0+ > +# Copyright (c) 2023 Linaro Limited > +# > +# Entry-type module for producing an empty EFI capsule > +# > + > +import os > + > +from binman.entry import Entry > +from binman.etype.section import Entry_section > +from dtoc import fdt_util > +from u_boot_pylib import tools > + > +class Entry_efi_empty_capsule(Entry_section):
Do you think this could subclass Entry_efi_capsule? They seem to do similar things. You could call generate_capsule() or generate_empty_capsule(). depending on whether any data is present (and perhaps require an operation if no data). I'm not sure about this, just an idea. > + """Generate EFI empty capsules > + > + The parameters needed for generation of the empty capsules can > + be provided as properties in the entry. > + > + Properties / Entry arguments: > + - image-guid: Image GUID which will be used for identifying the > + updatable image on the board. Mandatory for accept capsule. > + - accept-capsule - Boolean property to generate an accept capsule. > + image-type-id > + - revert-capsule - Boolean property to generate a revert capsule > + > + 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`_. For more information on the empty capsule, refer the > + sections 2.3.2 and 2.3.3 in the `Dependable Boot specification`_. > + > + A typical accept empty capsule entry node would then look something like > this > + > + empty-capsule { > + type = "efi-empty-capsule"; > + /* Image GUID for testing capsule update */ > + image-type-id = SANDBOX_UBOOT_IMAGE_GUID; > + accept-capsule; > + }; > + > + A typical revert empty capsule entry node would then look something like > this > + > + empty-capsule { > + type = "efi-empty-capsule"; > + revert-capsule; > + }; > + > + The empty capsules do not have any input payload image. > + > + .. _`UEFI specification`: > https://uefi.org/sites/default/files/resources/UEFI_Spec_2_10_Aug29.pdf > + .. _`Dependable Boot specification`: > https://git.codelinaro.org/linaro/dependable-boot/mbfw/uploads/6f7ddfe3be24e18d4319e108a758d02e/mbfw.pdf > + """ > + def __init__(self, section, etype, node): > + super().__init__(section, etype, node) > + self.accept = 0 > + self.revert = 0 > + > + def ReadNode(self): > + super().ReadNode() > + > + self.image_guid = fdt_util.GetString(self._node, 'image-guid') > + self.accept = fdt_util.GetBool(self._node, 'accept-capsule') > + self.revert = fdt_util.GetBool(self._node, 'revert-capsule') Perhaps it should be operation = "accept" / "revert" ? Using two conflicting bools is not great. > + > + if self.accept and not self.image_guid: > + self.Raise('Image GUID needed for generating accept capsule') > + > + if self.accept and self.revert: > + self.Raise('Need to enable either Accept or Revert capsule') > + > + def BuildSectionData(self, required): > + def get_binman_test_guid(type_str): > + TYPE_TO_GUID = { > + 'binman-test' : '09d7cf52-0720-4710-91d1-08469b7fe9c8' Can you put this in a shared file somewhere, used by this and the efi_capsule.py module? > + } > + return TYPE_TO_GUID[type_str] > + > + uniq = self.GetUniqueName() > + outfile = self._filename if self._filename else 'capsule.%s' % uniq > + capsule_fname = tools.get_output_filename(outfile) > + guid = self.image_guid > + if self.image_guid == "binman-test": > + guid = get_binman_test_guid('binman-test') > + > + ret = self.mkeficapsule.generate_empty_capsule(self.accept, > self.revert, > + guid, capsule_fname) > + if ret is not None: > + return tools.read_file(capsule_fname) > + > + def AddBintools(self, btools): > + self.mkeficapsule = self.AddBintool(btools, 'mkeficapsule') > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py > index 2b8871ab7e..9286de28e4 100644 > --- a/tools/binman/ftest.py > +++ b/tools/binman/ftest.py > @@ -126,6 +126,9 @@ FW_MGMT_GUID = '6DCBD5ED-E82D-4C44-BDA1-7194199AD92A' > CAPSULE_IMAGE_GUID = '09D7CF52-0720-4710-91D1-08469B7FE9C8' > # Windows cert GUID > WIN_CERT_TYPE_EFI_GUID = '4AAFD29D-68DF-49EE-8AA9-347D375665A7' > +# Empty capsule GUIDs > +EMPTY_CAPSULE_ACCEPT_GUID = '0C996046-BCC0-4D04-85EC-E1FCEDF1C6F8' > +EMPTY_CAPSULE_REVERT_GUID = 'ACD58B4B-C0E8-475F-99B5-6B3F7E07AAF0' > > class TestFunctional(unittest.TestCase): > """Functional tests for binman > @@ -7283,6 +7286,27 @@ fdt fdtmap Extract the > devicetree blob from the fdtmap > > self.assertEqual(payload_data_len, int(hdr['Payload Image Size'])) > > + def _CheckEmptyCapsule(self, data, accept_capsule=False): > + if accept_capsule: > + capsule_hdr_guid = EMPTY_CAPSULE_ACCEPT_GUID > + else: > + capsule_hdr_guid = EMPTY_CAPSULE_REVERT_GUID > + > + hdr = self._GetCapsuleHeaders(data) > + > + self.assertEqual(capsule_hdr_guid, > + hdr['EFI_CAPSULE_HDR.CAPSULE_GUID']) > + > + if accept_capsule: > + capsule_size = "0000002C" > + else: > + capsule_size = "0000001C" > + self.assertEqual(capsule_size, > + hdr['EFI_CAPSULE_HDR.CAPSULE_IMAGE_SIZE']) > + > + if accept_capsule: > + self.assertEqual(CAPSULE_IMAGE_GUID, hdr['ACCEPT_IMAGE_GUID']) > + > def testCapsuleGen(self): > """Test generation of EFI capsule""" > data = self._DoReadFile('311_capsule.dts') > @@ -7347,5 +7371,33 @@ fdt fdtmap Extract the > devicetree blob from the fdtmap > self.assertIn("entry is missing properties: image-guid", > str(e.exception)) > > + def testCapsuleGenAcceptCapsule(self): > + """Test generationg of accept EFI capsule""" > + data = self._DoReadFile('319_capsule_accept.dts') > + > + self._CheckEmptyCapsule(data, accept_capsule=True) > + > + def testCapsuleGenRevertCapsule(self): > + """Test generationg of revert EFI capsule""" > + data = self._DoReadFile('320_capsule_revert.dts') > + > + self._CheckEmptyCapsule(data) > + > + def testCapsuleGenAcceptGuidMissing(self): > + """Test that binman errors out on missing image GUID for accept > capsule""" > + with self.assertRaises(ValueError) as e: > + self._DoReadFile('321_capsule_accept_missing_guid.dts') > + > + self.assertIn("Image GUID needed for generating accept capsule", > + str(e.exception)) > + > + def testCapsuleGenAcceptOrRevert(self): > + """Test that both accept and revert capsule are not specified""" > + with self.assertRaises(ValueError) as e: > + self._DoReadFile('322_capsule_accept_revert.dts') > + > + self.assertIn("Need to enable either Accept or Revert capsule", > + str(e.exception)) > + > if __name__ == "__main__": > unittest.main() > diff --git a/tools/binman/test/319_capsule_accept.dts > b/tools/binman/test/319_capsule_accept.dts > new file mode 100644 > index 0000000000..4d6c005019 > --- /dev/null > +++ b/tools/binman/test/319_capsule_accept.dts > @@ -0,0 +1,16 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +/dts-v1/; > + > +/ { > + #address-cells = <1>; > + #size-cells = <1>; > + > + binman { > + efi-empty-capsule { > + /* Image GUID for testing capsule update */ > + image-guid = "binman-test"; > + accept-capsule; > + }; > + }; > +}; > diff --git a/tools/binman/test/320_capsule_revert.dts > b/tools/binman/test/320_capsule_revert.dts > new file mode 100644 > index 0000000000..eeaa2793a5 > --- /dev/null > +++ b/tools/binman/test/320_capsule_revert.dts > @@ -0,0 +1,14 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +/dts-v1/; > + > +/ { > + #address-cells = <1>; > + #size-cells = <1>; > + > + binman { > + efi-empty-capsule { > + revert-capsule; > + }; > + }; > +}; > diff --git a/tools/binman/test/321_capsule_accept_missing_guid.dts > b/tools/binman/test/321_capsule_accept_missing_guid.dts > new file mode 100644 > index 0000000000..6f7062e83e > --- /dev/null > +++ b/tools/binman/test/321_capsule_accept_missing_guid.dts > @@ -0,0 +1,14 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +/dts-v1/; > + > +/ { > + #address-cells = <1>; > + #size-cells = <1>; I wonder if those two lines are needed? Perhaps it generates a warning? > + > + binman { > + efi-empty-capsule { > + accept-capsule; > + }; > + }; > +}; > diff --git a/tools/binman/test/322_capsule_accept_revert.dts > b/tools/binman/test/322_capsule_accept_revert.dts > new file mode 100644 > index 0000000000..c68e76a669 > --- /dev/null > +++ b/tools/binman/test/322_capsule_accept_revert.dts > @@ -0,0 +1,17 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +/dts-v1/; > + > +/ { > + #address-cells = <1>; > + #size-cells = <1>; > + > + binman { > + efi-empty-capsule { > + /* Image GUID for testing capsule update */ > + image-guid = "binman-test"; > + accept-capsule; > + revert-capsule; > + }; > + }; > +}; > -- > 2.34.1 > Regards, Simon