hi Simon, On Sun, 8 Oct 2023 at 04:41, Simon Glass <s...@chromium.org> wrote: > > 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.
Okay, let me try this out. If it is getting too confusing or unclear, I will keep the two entry types separate. > > > + """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. Will change > > > + > > + 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? Okay. I will put this in a separate global function, and use it in the other 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? Tbh, I just copied them from another file which I was using as a reference when I introduced the capsule changes. I will check if removing them causes any issues. -sughosh > > > + > > + 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