| Message ID | 20230715134533.2025893-7-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Integrate EFI capsule tasks into u-boot's build flow | expand |
Hi Sughosh, On Sat, 15 Jul 2023 at 07:46, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > Add support in binman for generating capsules. The capsule parameters > can be specified either through a config file or through the capsule > binman entry. Also add test cases in binman for capsule generation, > and enable this testing on the sandbox_spl variant. Can you use sandbox instead, or perhaps sandbox_spl? SPL is really for SPL testing. > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > --- > Changes since V3: > * Add test cases for covering the various capsule generation > scenarios. > * Add function comments in the mkeficapsule bintool. > * Fix the fetch method of the mkeficapsule bintool to enable building > the tool. > * Add more details about the capsule parameters in the documentation > as well as the code. > * Fix order of module imports, and addition of blank lines in the > capsule.py file. > * Use SetContents in the ObtainContents method. > > configs/sandbox_spl_defconfig | 1 + > tools/binman/btool/mkeficapsule.py | 158 ++++++++++++++++++ > tools/binman/entries.rst | 37 ++++ > tools/binman/etype/capsule.py | 132 +++++++++++++++ > tools/binman/ftest.py | 127 ++++++++++++++ > tools/binman/test/282_capsule.dts | 18 ++ > tools/binman/test/283_capsule_signed.dts | 20 +++ > tools/binman/test/284_capsule_conf.dts | 14 ++ > tools/binman/test/285_capsule_missing_key.dts | 19 +++ > .../binman/test/286_capsule_missing_index.dts | 17 ++ > .../binman/test/287_capsule_missing_guid.dts | 17 ++ > .../test/288_capsule_missing_payload.dts | 17 ++ > tools/binman/test/289_capsule_missing.dts | 17 ++ > tools/binman/test/290_capsule_version.dts | 19 +++ > tools/binman/test/capsule_cfg.txt | 6 + > 15 files changed, 619 insertions(+) > create mode 100644 tools/binman/btool/mkeficapsule.py > create mode 100644 tools/binman/etype/capsule.py > create mode 100644 tools/binman/test/282_capsule.dts > create mode 100644 tools/binman/test/283_capsule_signed.dts > create mode 100644 tools/binman/test/284_capsule_conf.dts > create mode 100644 tools/binman/test/285_capsule_missing_key.dts > create mode 100644 tools/binman/test/286_capsule_missing_index.dts > create mode 100644 tools/binman/test/287_capsule_missing_guid.dts > create mode 100644 tools/binman/test/288_capsule_missing_payload.dts > create mode 100644 tools/binman/test/289_capsule_missing.dts > create mode 100644 tools/binman/test/290_capsule_version.dts > create mode 100644 tools/binman/test/capsule_cfg.txt This looks pretty good to me. Some nits below > > diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig > index dd848c57c6..2fcc789347 100644 > --- a/configs/sandbox_spl_defconfig > +++ b/configs/sandbox_spl_defconfig > @@ -248,3 +248,4 @@ CONFIG_UNIT_TEST=y > CONFIG_SPL_UNIT_TEST=y > CONFIG_UT_TIME=y > CONFIG_UT_DM=y > +CONFIG_TOOLS_MKEFICAPSULE=y Why enabling this here? I don't think it is needed in sandbox_spl, but in any case it should be in a different patch if needed. > diff --git a/tools/binman/btool/mkeficapsule.py b/tools/binman/btool/mkeficapsule.py > new file mode 100644 > index 0000000000..ba6b666714 > --- /dev/null > +++ b/tools/binman/btool/mkeficapsule.py > @@ -0,0 +1,158 @@ > +# 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 > + -f, --cfg-file <config file> config file with capsule parameters > + -h, --help print a help message > + > +""" > + > +from binman import bintool > + > +class Bintoolmkeficapsule(bintool.Bintool): > + """Handles the 'mkeficapsule' tool > + > + This bintool is used for generating the EFI capsules. The > + capsule generation parameters can either be specified through > + command-line, or through a config file. > + > + """ > + def __init__(self, name): > + super().__init__(name, 'mkeficapsule tool for generating capsules') > + > + def capsule_cfg_file(self, cfg_file): > + """Generate a capsule reading parameters from config file > + > + Args: > + cfg_file (str): Path to the config file > + > + Returns: > + str: Tool output > + """ > + nit: drop blank lines after """ function comment (please fix throughout) > + args = [ > + f'--cfg-file={cfg_file}' > + ] > + return self.run_cmd(*args) > + > + def cmdline_capsule(self, image_index, image_guid, hardware_instance, > + payload, output_fname, version=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 > + version (int): Image version (Optional) > + > + Returns: > + str: Tool output > + """ > + > + if version: > + args = [ > + f'--index={image_index}', > + f'--fw-version={version}', > + f'--guid={image_guid}', > + f'--instance={hardware_instance}', > + payload, > + output_fname > + ] > + else: > + args = [ > + f'--index={image_index}', > + f'--guid={image_guid}', > + f'--instance={hardware_instance}', > + payload, > + output_fname > + ] > + > + return self.run_cmd(*args) > + > + def cmdline_auth_capsule(self, image_index, image_guid, hardware_instance, > + monotonic_count, priv_key, pub_key, > + payload, output_fname, version=0): > + """Generate a signed 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 > + monotonic_count (int): Count used when signing an image > + priv_key (str): Path to the private key > + pub_key(str): Path to the public key > + payload (str): Path to the input payload image > + output_fname (str): Path to the output capsule file > + version (int): Image version (Optional) > + > + Returns: > + str: Tool output > + """ > + > + if version: > + args = [ > + f'--index={image_index}', > + f'--guid={image_guid}', > + f'--instance={hardware_instance}', > + f'--monotonic-count={monotonic_count}', > + f'--private-key={priv_key}', > + f'--certificate={pub_key}', > + f'--fw-version={version}', > + payload, > + output_fname > + ] > + else: > + args = [ > + f'--index={image_index}', > + f'--guid={image_guid}', > + f'--instance={hardware_instance}', > + f'--monotonic-count={monotonic_count}', > + f'--private-key={priv_key}', > + f'--certificate={pub_key}', > + payload, > + output_fname > + ] > + > + 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 b71af801fd..523c8222f5 100644 > --- a/tools/binman/entries.rst > +++ b/tools/binman/entries.rst > @@ -283,6 +283,43 @@ entry; similarly for SPL. > > > > +.. _etype_capsule: > + > +Entry: capsule: Entry for generating EFI Capsule files > +------------------------------------------------------ > + > +This is an entry for generating EFI capsules. > + > +The parameters needed for generation of the capsules can either be > +provided separately, or through a config file. > + > +Properties / Entry arguments: > + - cfg-file: Config file for providing capsule > + parameters. These are parameters needed for generating the > + capsules. The parameters can be listed by running the > + './tools/mkeficapsule -h' command. > + - 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. > + - monotomic-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. > + - filename: Path to the input(payload) file. File can be any > + format, a binary or an elf, platform specific. > + - capsule: Path to the output capsule file. A capsule is a > + continous set of data as defined by the EFI specification. Refer > + to the specification for more details. > + > + > + > .. _etype_cbfs: > > Entry: cbfs: Coreboot Filesystem (CBFS) > diff --git a/tools/binman/etype/capsule.py b/tools/binman/etype/capsule.py > new file mode 100644 > index 0000000000..46e5507704 > --- /dev/null > +++ b/tools/binman/etype/capsule.py > @@ -0,0 +1,132 @@ > +# SPDX-License-Identifier: GPL-2.0+ > +# Copyright (c) 2023 Linaro Limited > +# > +# Entry-type module for producing a capsule > +# > + > +import os > + > +from binman.entry import Entry > +from dtoc import fdt_util > +from u_boot_pylib import tools > + > +class Entry_capsule(Entry): > + """Entry for generating EFI capsules > + > + This is an entry for generating EFI capsules. > + > + The parameters needed for generation of the capsules can > + either be provided separately, or through a config file. > + > + Properties / Entry arguments: > + - cfg-file: Config file for providing capsule > + parameters. These are parameters needed for generating the > + capsules. The parameters can be listed by running the > + './tools/mkeficapsule -h' command. > + - 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. > + - monotomic-count: Count used when signing an image. monotonic > + - private-key: Path to PEM formatted .key private key file. > + - pub-key-cert: Path to PEM formatted .crt public key certificate > + file. > + - filename: Path to the input(payload) file. File can be any > + format, a binary or an elf, platform specific. > + - capsule: Path to the output capsule file. A capsule is a > + continous set of data as defined by the EFI specification. Refer continuous Can you add a link to EFI spec so it appears in the docs here? > + to the specification for more details. > + > + """ > + def __init__(self, section, etype, node): > + super().__init__(section, etype, node) > + self.image_index = 0 > + self.image_guid = "" > + self.hardware_instance = 0 > + self.monotonic_count = 0 > + self.fw_version = 0 > + self.private_key = "" > + self.pub_key_cert = "" > + self.auth = 0 > + self.payload = "" > + self.capsule_fname = "" Please remember to use single quotes > + > + def ReadNode(self): > + super().ReadNode() > + > + self.cfg_file = fdt_util.GetString(self._node, 'cfg-file') > + if not self.cfg_file: > + self.image_index = fdt_util.GetInt(self._node, 'image-index') > + if not self.image_index: > + self.Raise('mkeficapsule must be provided an Image Index') > + > + self.image_guid = fdt_util.GetString(self._node, 'image-type-id') > + if not self.image_guid: > + self.Raise('mkeficapsule must be provided an Image GUID') Use self.required_props = ['image-type-id', ...] in your __init__(). Then this is automatic > + > + 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.private_key = fdt_util.GetString(self._node, 'private-key') > + self.pub_key_cert = fdt_util.GetString(self._node, 'pub-key-cert') > + > + if ((self.private_key and not self.pub_key_cert) or (self.pub_key_cert and not self.private_key)): > + self.Raise('Both private-key and public key Certificate need to be provided') > + elif not (self.private_key and self.pub_key_cert): > + self.auth = 0 > + else: > + self.auth = 1 > + > + self.payload = fdt_util.GetString(self._node, 'filename') > + if not self.payload: > + self.Raise('mkeficapsule must be provided an input filename(payload)') > + > + if not os.path.isabs(self.payload): > + self.payload_path = tools.get_input_filename(self.payload) > + if not os.path.exists(self.payload_path): > + self.Raise('Cannot resolve path to the input filename(payload)') > + else: > + self.payload = self.payload_path > + > + self.capsule_fname = fdt_util.GetString(self._node, 'capsule') > + if not self.capsule_fname: > + self.Raise('Specify the output capsule file') > + > + if not os.path.isabs(self.capsule_fname): > + self.capsule_path = tools.get_output_filename(self.capsule_fname) > + self.capsule_fname = self.capsule_path > + > + def _GenCapsule(self): What if some of the inputs are missing? Does this handle missing blobs? > + if self.cfg_file: > + return self.mkeficapsule.capsule_cfg_file(self.cfg_file) > + elif self.auth: > + return self.mkeficapsule.cmdline_auth_capsule(self.image_index, > + self.image_guid, > + self.hardware_instance, > + self.monotonic_count, > + self.private_key, > + self.pub_key_cert, > + self.payload, > + self.capsule_fname, > + self.fw_version) > + else: > + return self.mkeficapsule.cmdline_capsule(self.image_index, > + self.image_guid, > + self.hardware_instance, > + self.payload, > + self.capsule_fname, > + self.fw_version) > + > + def ObtainContents(self): > + self.SetContents(tools.to_bytes(self._GenCapsule())) > + return True > + > + def AddBintools(self, btools): > + self.mkeficapsule = self.AddBintool(btools, 'mkeficapsule') > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py > index 43b4f850a6..f5dd62d028 100644 > --- a/tools/binman/ftest.py > +++ b/tools/binman/ftest.py > @@ -6676,6 +6676,133 @@ fdt fdtmap Extract the devicetree blob from the fdtmap > ['fit']) > self.assertIn("Node '/fit': Missing tool: 'mkimage'", str(e.exception)) > > + def _CheckCapsule(self, signed_capsule=False, version_check=False): > + # Firmware Management Protocol GUID used in capsule header > + self.capsule_guid = "edd5cb6d2de8444cbda17194199ad92a" Please add a constant FW_MGMT_GUID = '' at the top of the file We really should not have GUIDs in the code...they are a mess. > + # Image GUID specified in the DTS > + self.image_guid = "52cfd7092007104791d108469b7fe9c8" > + self.fmp_signature = "4d535331" > + self.fmp_size = "10" > + self.fmp_fw_version = "02" These should really be local vars, not members. > + > + self.capsule_data = tools.read_file(self.capsule_fname) Pass the data in here and then you don't need to read the file > + > + self.assertEqual(self.capsule_guid, self.capsule_data.hex()[:32]) > + self.assertEqual(self.image_guid, self.capsule_data.hex()[96:128]) > + > + if version_check: > + self.assertEqual(self.fmp_signature, self.capsule_data.hex()[184:192]) > + self.assertEqual(self.fmp_size, self.capsule_data.hex()[192:194]) > + self.assertEqual(self.fmp_fw_version, self.capsule_data.hex()[200:202]) > + > + if signed_capsule: > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[4770:4778]) Where do these integer offsets come from? Please add a comment > + elif version_check: > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[216:224]) > + else: > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[184:192]) > + > + def _GenCapsuleCfgPayload(self, fname, contents): > + capsule_dir = '/tmp/capsules/' You can't write to /tmp Please use self._indir for input files - see how other tests do it > + pathname = os.path.join(capsule_dir, fname) > + dirname = os.path.dirname(pathname) > + if dirname and not os.path.exists(dirname): > + os.makedirs(dirname) > + > + with open(pathname, 'wb') as fd: > + fd.write(contents) tools.write_file(pathname, contents) > + > + def testCapsuleGen(self): > + """Test generation of EFI capsule""" > + self.payload_data = tools.to_bytes(TEXT_DATA) Please can you use your own test data, like EFI_DATA ? Also if you declare it as a binary string you can drop the call. For example: EFI_DATA = b'efi' > + > + TestFunctional._MakeInputFile('payload.txt', self.payload_data) > + > + self._DoReadFile('282_capsule.dts') data = self... > + > + self.capsule_fname = tools.get_output_filename('test.capsule') > + self.assertTrue(os.path.exists(self.capsule_fname)) You can drop that line > + > + self._CheckCapsule() self._CheckCapsule(data) > + > + def testSignedCapsuleGen(self): > + """Test generation of EFI capsule""" > + self.payload_data = tools.to_bytes(TEXT_DATA) > + > + TestFunctional._MakeInputFile('payload.txt', self.payload_data) > + > + self._DoReadFile('283_capsule_signed.dts') data = self... That is the actual data, so you don't need to read it below. > + > + self.capsule_fname = tools.get_output_filename('test.capsule') > + self.assertTrue(os.path.exists(self.capsule_fname)) > + > + self._CheckCapsule(signed_capsule=True) > + > + def testCapsuleGenCfgFile(self): > + """Test generation of EFI capsule through config file""" > + self.payload_data = tools.to_bytes(TEXT_DATA) > + > + self._GenCapsuleCfgPayload('payload.txt', self.payload_data) > + > + self._DoReadFile('284_capsule_conf.dts') > + > + self.capsule_fname = '/tmp/capsules/test.capsule' > + self.assertTrue(os.path.exists(self.capsule_fname)) > + > + self._CheckCapsule() > + > + def testCapsuleGenVersionSupport(self): > + """Test generation of EFI capsule with version support""" > + self.payload_data = tools.to_bytes(TEXT_DATA) > > + TestFunctional._MakeInputFile('payload.txt', self.payload_data) > + > + self._DoReadFile('290_capsule_version.dts') > + > + self.capsule_fname = tools.get_output_filename('test.capsule') > + self.assertTrue(os.path.exists(self.capsule_fname)) > + > + self._CheckCapsule(version_check=True) > + > + def testCapsuleGenKeyMissing(self): > + """Test that binman errors out on missing key""" > + with self.assertRaises(ValueError) as e: > + self._DoReadFile('285_capsule_missing_key.dts') > + > + self.assertIn("Both private-key and public key Certificate need to be provided", private key certificate > + str(e.exception)) > + > + def testCapsuleGenIndexMissing(self): > + """Test that binman errors out on missing image index""" > + with self.assertRaises(ValueError) as e: > + self._DoReadFile('286_capsule_missing_index.dts') > + > + self.assertIn("mkeficapsule must be provided an 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('287_capsule_missing_guid.dts') > + > + self.assertIn("mkeficapsule must be provided an Image GUID", > + str(e.exception)) > + > + def testCapsuleGenPayloadMissing(self): > + """Test that binman errors out on missing input(payload)image""" > + with self.assertRaises(ValueError) as e: > + self._DoReadFile('288_capsule_missing_payload.dts') > + > + self.assertIn("mkeficapsule must be provided an input filename(payload)", > + str(e.exception)) > + > + def testCapsuleGenCapsuleFileMissing(self): > + """Test that binman errors out on missing output capsule file""" > + with self.assertRaises(ValueError) as e: > + self._DoReadFile('289_capsule_missing.dts') > + > + self.assertIn("Specify the output capsule file", > + str(e.exception)) This looks good. It is a pain that you need to cover each missing arg. I'm not sure I can think of a better way. > + > if __name__ == "__main__": > unittest.main() > diff --git a/tools/binman/test/282_capsule.dts b/tools/binman/test/282_capsule.dts > new file mode 100644 > index 0000000000..0e7fcdf8ab > --- /dev/null > +++ b/tools/binman/test/282_capsule.dts > @@ -0,0 +1,18 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +/dts-v1/; > + > +/ { > + #address-cells = <1>; > + #size-cells = <1>; > + > + binman { > + capsule { > + image-index = <0x1>; > + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; Is there a #define somewhere for this? Perhaps you can add a #define, or a comment as to what this is. Also, please use lower case. > + hardware-instance = <0x0>; > + filename = "payload.txt"; > + capsule = "test.capsule"; > + }; > + }; > +}; Regards, Simon
hi Simon, On Sun, 16 Jul 2023 at 05:12, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Sat, 15 Jul 2023 at 07:46, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > Add support in binman for generating capsules. The capsule parameters > > can be specified either through a config file or through the capsule > > binman entry. Also add test cases in binman for capsule generation, > > and enable this testing on the sandbox_spl variant. > > Can you use sandbox instead, or perhaps sandbox_spl? SPL is really for > SPL testing. Er, I am actually using the sandbox_spl variant. > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > --- > > Changes since V3: > > * Add test cases for covering the various capsule generation > > scenarios. > > * Add function comments in the mkeficapsule bintool. > > * Fix the fetch method of the mkeficapsule bintool to enable building > > the tool. > > * Add more details about the capsule parameters in the documentation > > as well as the code. > > * Fix order of module imports, and addition of blank lines in the > > capsule.py file. > > * Use SetContents in the ObtainContents method. > > > > configs/sandbox_spl_defconfig | 1 + > > tools/binman/btool/mkeficapsule.py | 158 ++++++++++++++++++ > > tools/binman/entries.rst | 37 ++++ > > tools/binman/etype/capsule.py | 132 +++++++++++++++ > > tools/binman/ftest.py | 127 ++++++++++++++ > > tools/binman/test/282_capsule.dts | 18 ++ > > tools/binman/test/283_capsule_signed.dts | 20 +++ > > tools/binman/test/284_capsule_conf.dts | 14 ++ > > tools/binman/test/285_capsule_missing_key.dts | 19 +++ > > .../binman/test/286_capsule_missing_index.dts | 17 ++ > > .../binman/test/287_capsule_missing_guid.dts | 17 ++ > > .../test/288_capsule_missing_payload.dts | 17 ++ > > tools/binman/test/289_capsule_missing.dts | 17 ++ > > tools/binman/test/290_capsule_version.dts | 19 +++ > > tools/binman/test/capsule_cfg.txt | 6 + > > 15 files changed, 619 insertions(+) > > create mode 100644 tools/binman/btool/mkeficapsule.py > > create mode 100644 tools/binman/etype/capsule.py > > create mode 100644 tools/binman/test/282_capsule.dts > > create mode 100644 tools/binman/test/283_capsule_signed.dts > > create mode 100644 tools/binman/test/284_capsule_conf.dts > > create mode 100644 tools/binman/test/285_capsule_missing_key.dts > > create mode 100644 tools/binman/test/286_capsule_missing_index.dts > > create mode 100644 tools/binman/test/287_capsule_missing_guid.dts > > create mode 100644 tools/binman/test/288_capsule_missing_payload.dts > > create mode 100644 tools/binman/test/289_capsule_missing.dts > > create mode 100644 tools/binman/test/290_capsule_version.dts > > create mode 100644 tools/binman/test/capsule_cfg.txt > > This looks pretty good to me. Some nits below > > > > > diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig > > index dd848c57c6..2fcc789347 100644 > > --- a/configs/sandbox_spl_defconfig > > +++ b/configs/sandbox_spl_defconfig > > @@ -248,3 +248,4 @@ CONFIG_UNIT_TEST=y > > CONFIG_SPL_UNIT_TEST=y > > CONFIG_UT_TIME=y > > CONFIG_UT_DM=y > > +CONFIG_TOOLS_MKEFICAPSULE=y > > Why enabling this here? I don't think it is needed in sandbox_spl, but > in any case it should be in a different patch if needed. The binman tests run on the sandbox_spl variant. When running the capsule generation tests, the mkeficapsule tool should be present on the board variant no? > > > diff --git a/tools/binman/btool/mkeficapsule.py b/tools/binman/btool/mkeficapsule.py > > new file mode 100644 > > index 0000000000..ba6b666714 > > --- /dev/null > > +++ b/tools/binman/btool/mkeficapsule.py > > @@ -0,0 +1,158 @@ > > +# 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 > > + -f, --cfg-file <config file> config file with capsule parameters > > + -h, --help print a help message > > + > > +""" > > + > > +from binman import bintool > > + > > +class Bintoolmkeficapsule(bintool.Bintool): > > + """Handles the 'mkeficapsule' tool > > + > > + This bintool is used for generating the EFI capsules. The > > + capsule generation parameters can either be specified through > > + command-line, or through a config file. > > + > > + """ > > + def __init__(self, name): > > + super().__init__(name, 'mkeficapsule tool for generating capsules') > > + > > + def capsule_cfg_file(self, cfg_file): > > + """Generate a capsule reading parameters from config file > > + > > + Args: > > + cfg_file (str): Path to the config file > > + > > + Returns: > > + str: Tool output > > + """ > > + > > nit: drop blank lines after """ function comment (please fix throughout) Okay > > > + args = [ > > + f'--cfg-file={cfg_file}' > > + ] > > + return self.run_cmd(*args) > > + > > + def cmdline_capsule(self, image_index, image_guid, hardware_instance, > > + payload, output_fname, version=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 > > + version (int): Image version (Optional) > > + > > + Returns: > > + str: Tool output > > + """ > > + > > + if version: > > + args = [ > > + f'--index={image_index}', > > + f'--fw-version={version}', > > + f'--guid={image_guid}', > > + f'--instance={hardware_instance}', > > + payload, > > + output_fname > > + ] > > + else: > > + args = [ > > + f'--index={image_index}', > > + f'--guid={image_guid}', > > + f'--instance={hardware_instance}', > > + payload, > > + output_fname > > + ] > > + > > + return self.run_cmd(*args) > > + > > + def cmdline_auth_capsule(self, image_index, image_guid, hardware_instance, > > + monotonic_count, priv_key, pub_key, > > + payload, output_fname, version=0): > > + """Generate a signed 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 > > + monotonic_count (int): Count used when signing an image > > + priv_key (str): Path to the private key > > + pub_key(str): Path to the public key > > + payload (str): Path to the input payload image > > + output_fname (str): Path to the output capsule file > > + version (int): Image version (Optional) > > + > > + Returns: > > + str: Tool output > > + """ > > + > > + if version: > > + args = [ > > + f'--index={image_index}', > > + f'--guid={image_guid}', > > + f'--instance={hardware_instance}', > > + f'--monotonic-count={monotonic_count}', > > + f'--private-key={priv_key}', > > + f'--certificate={pub_key}', > > + f'--fw-version={version}', > > + payload, > > + output_fname > > + ] > > + else: > > + args = [ > > + f'--index={image_index}', > > + f'--guid={image_guid}', > > + f'--instance={hardware_instance}', > > + f'--monotonic-count={monotonic_count}', > > + f'--private-key={priv_key}', > > + f'--certificate={pub_key}', > > + payload, > > + output_fname > > + ] > > + > > + 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 b71af801fd..523c8222f5 100644 > > --- a/tools/binman/entries.rst > > +++ b/tools/binman/entries.rst > > @@ -283,6 +283,43 @@ entry; similarly for SPL. > > > > > > > > +.. _etype_capsule: > > + > > +Entry: capsule: Entry for generating EFI Capsule files > > +------------------------------------------------------ > > + > > +This is an entry for generating EFI capsules. > > + > > +The parameters needed for generation of the capsules can either be > > +provided separately, or through a config file. > > + > > +Properties / Entry arguments: > > + - cfg-file: Config file for providing capsule > > + parameters. These are parameters needed for generating the > > + capsules. The parameters can be listed by running the > > + './tools/mkeficapsule -h' command. > > + - 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. > > + - monotomic-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. > > + - filename: Path to the input(payload) file. File can be any > > + format, a binary or an elf, platform specific. > > + - capsule: Path to the output capsule file. A capsule is a > > + continous set of data as defined by the EFI specification. Refer > > + to the specification for more details. > > + > > + > > + > > .. _etype_cbfs: > > > > Entry: cbfs: Coreboot Filesystem (CBFS) > > diff --git a/tools/binman/etype/capsule.py b/tools/binman/etype/capsule.py > > new file mode 100644 > > index 0000000000..46e5507704 > > --- /dev/null > > +++ b/tools/binman/etype/capsule.py > > @@ -0,0 +1,132 @@ > > +# SPDX-License-Identifier: GPL-2.0+ > > +# Copyright (c) 2023 Linaro Limited > > +# > > +# Entry-type module for producing a capsule > > +# > > + > > +import os > > + > > +from binman.entry import Entry > > +from dtoc import fdt_util > > +from u_boot_pylib import tools > > + > > +class Entry_capsule(Entry): > > + """Entry for generating EFI capsules > > + > > + This is an entry for generating EFI capsules. > > + > > + The parameters needed for generation of the capsules can > > + either be provided separately, or through a config file. > > + > > + Properties / Entry arguments: > > + - cfg-file: Config file for providing capsule > > + parameters. These are parameters needed for generating the > > + capsules. The parameters can be listed by running the > > + './tools/mkeficapsule -h' command. > > + - 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. > > + - monotomic-count: Count used when signing an image. > > monotonic > > > + - private-key: Path to PEM formatted .key private key file. > > + - pub-key-cert: Path to PEM formatted .crt public key certificate > > + file. > > + - filename: Path to the input(payload) file. File can be any > > + format, a binary or an elf, platform specific. > > + - capsule: Path to the output capsule file. A capsule is a > > + continous set of data as defined by the EFI specification. Refer > > continuous > > Can you add a link to EFI spec so it appears in the docs here? Will fix the above typos and add the link. > > > + to the specification for more details. > > + > > + """ > > + def __init__(self, section, etype, node): > > + super().__init__(section, etype, node) > > + self.image_index = 0 > > + self.image_guid = "" > > + self.hardware_instance = 0 > > + self.monotonic_count = 0 > > + self.fw_version = 0 > > + self.private_key = "" > > + self.pub_key_cert = "" > > + self.auth = 0 > > + self.payload = "" > > + self.capsule_fname = "" > > Please remember to use single quotes Okay > > > + > > + def ReadNode(self): > > + super().ReadNode() > > + > > + self.cfg_file = fdt_util.GetString(self._node, 'cfg-file') > > + if not self.cfg_file: > > + self.image_index = fdt_util.GetInt(self._node, 'image-index') > > + if not self.image_index: > > + self.Raise('mkeficapsule must be provided an Image Index') > > + > > + self.image_guid = fdt_util.GetString(self._node, 'image-type-id') > > + if not self.image_guid: > > + self.Raise('mkeficapsule must be provided an Image GUID') > > Use self.required_props = ['image-type-id', ...] in your __init__(). > Then this is automatic I should have clarified this during the earlier version itself. So these parameters are mandatory only when not using the config file. In the scenario of generating the capsules through config files, all these parameters are provided through the config file. Hence these explicit checks. > > > + > > + 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.private_key = fdt_util.GetString(self._node, 'private-key') > > + self.pub_key_cert = fdt_util.GetString(self._node, 'pub-key-cert') > > + > > + if ((self.private_key and not self.pub_key_cert) or (self.pub_key_cert and not self.private_key)): > > + self.Raise('Both private-key and public key Certificate need to be provided') > > + elif not (self.private_key and self.pub_key_cert): > > + self.auth = 0 > > + else: > > + self.auth = 1 > > + > > + self.payload = fdt_util.GetString(self._node, 'filename') > > + if not self.payload: > > + self.Raise('mkeficapsule must be provided an input filename(payload)') > > + > > + if not os.path.isabs(self.payload): > > + self.payload_path = tools.get_input_filename(self.payload) > > + if not os.path.exists(self.payload_path): > > + self.Raise('Cannot resolve path to the input filename(payload)') > > + else: > > + self.payload = self.payload_path > > + > > + self.capsule_fname = fdt_util.GetString(self._node, 'capsule') > > + if not self.capsule_fname: > > + self.Raise('Specify the output capsule file') > > + > > + if not os.path.isabs(self.capsule_fname): > > + self.capsule_path = tools.get_output_filename(self.capsule_fname) > > + self.capsule_fname = self.capsule_path > > + > > + def _GenCapsule(self): > > What if some of the inputs are missing? Does this handle missing blobs? Any missing input parameters are checked earlier itself. > > + if self.cfg_file: > > + return self.mkeficapsule.capsule_cfg_file(self.cfg_file) > > + elif self.auth: > > + return self.mkeficapsule.cmdline_auth_capsule(self.image_index, > > + self.image_guid, > > + self.hardware_instance, > > + self.monotonic_count, > > + self.private_key, > > + self.pub_key_cert, > > + self.payload, > > + self.capsule_fname, > > + self.fw_version) > > + else: > > + return self.mkeficapsule.cmdline_capsule(self.image_index, > > + self.image_guid, > > + self.hardware_instance, > > + self.payload, > > + self.capsule_fname, > > + self.fw_version) > > + > > + def ObtainContents(self): > > + self.SetContents(tools.to_bytes(self._GenCapsule())) > > + return True > > + > > + def AddBintools(self, btools): > > + self.mkeficapsule = self.AddBintool(btools, 'mkeficapsule') > > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py > > index 43b4f850a6..f5dd62d028 100644 > > --- a/tools/binman/ftest.py > > +++ b/tools/binman/ftest.py > > @@ -6676,6 +6676,133 @@ fdt fdtmap Extract the devicetree blob from the fdtmap > > ['fit']) > > self.assertIn("Node '/fit': Missing tool: 'mkimage'", str(e.exception)) > > > > + def _CheckCapsule(self, signed_capsule=False, version_check=False): > > + # Firmware Management Protocol GUID used in capsule header > > + self.capsule_guid = "edd5cb6d2de8444cbda17194199ad92a" > > Please add a constant FW_MGMT_GUID = '' at the top of the file Okay. Will add for the capsule_guid and image_guid values. > > We really should not have GUIDs in the code...they are a mess. You want the UEFI capsule generation to happen through binman, and not mention GUIDs. That ain't happening :) > > > + # Image GUID specified in the DTS > > + self.image_guid = "52cfd7092007104791d108469b7fe9c8" > > + self.fmp_signature = "4d535331" > > + self.fmp_size = "10" > > + self.fmp_fw_version = "02" > > These should really be local vars, not members. Okay > > > + > > + self.capsule_data = tools.read_file(self.capsule_fname) > > Pass the data in here and then you don't need to read the file So the file needs to be read here since the actual capsule generation tool(tools/mkeficapsule) does not return any capsule data. Instead, the data gets written to the capsule file, and the tool just returns a pass/fail status. > > > + > > + self.assertEqual(self.capsule_guid, self.capsule_data.hex()[:32]) > > + self.assertEqual(self.image_guid, self.capsule_data.hex()[96:128]) > > + > > + if version_check: > > + self.assertEqual(self.fmp_signature, self.capsule_data.hex()[184:192]) > > + self.assertEqual(self.fmp_size, self.capsule_data.hex()[192:194]) > > + self.assertEqual(self.fmp_fw_version, self.capsule_data.hex()[200:202]) > > + > > + if signed_capsule: > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[4770:4778]) > > Where do these integer offsets come from? Please add a comment So, these are simply offsets in the output capsule file, which get impacted based on the contents being put in the capsule, like presence/absence of optional headers. I don't think putting a comment really wll add any value, because these offsets are variable. > > > + elif version_check: > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[216:224]) > > + else: > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[184:192]) > > + > > + def _GenCapsuleCfgPayload(self, fname, contents): > > + capsule_dir = '/tmp/capsules/' > > You can't write to /tmp > > Please use self._indir for input files - see how other tests do it For all other tests, I am indeed using _indir and outdir. But for generation of capsules through a config file, we need to specify the location where the output capsule file will be written to. Which is the reason for the /tmp/capsules/. We are using this directory for collating all capsule related files. > > > + pathname = os.path.join(capsule_dir, fname) > > + dirname = os.path.dirname(pathname) > > + if dirname and not os.path.exists(dirname): > > + os.makedirs(dirname) > > + > > + with open(pathname, 'wb') as fd: > > + fd.write(contents) > > tools.write_file(pathname, contents) Okay > > > + > > + def testCapsuleGen(self): > > + """Test generation of EFI capsule""" > > + self.payload_data = tools.to_bytes(TEXT_DATA) > > Please can you use your own test data, like EFI_DATA ? Also if you > declare it as a binary string you can drop the call. > > For example: > > EFI_DATA = b'efi' I don't know why text_data cannot be used, but I will add the EFI_DATA. > > > + > > + TestFunctional._MakeInputFile('payload.txt', self.payload_data) > > + > > + self._DoReadFile('282_capsule.dts') > > data = self... Please see above. We need to read the capsule file. This applies for all the related comments about using the data = self._DoReadFile... > > + > > + self.capsule_fname = tools.get_output_filename('test.capsule') > > + self.assertTrue(os.path.exists(self.capsule_fname)) > > You can drop that line > > > + > > + self._CheckCapsule() > > self._CheckCapsule(data) > > > + > > + def testSignedCapsuleGen(self): > > + """Test generation of EFI capsule""" > > + self.payload_data = tools.to_bytes(TEXT_DATA) > > + > > + TestFunctional._MakeInputFile('payload.txt', self.payload_data) > > + > > + self._DoReadFile('283_capsule_signed.dts') > > data = self... > > That is the actual data, so you don't need to read it below. > > > + > > + self.capsule_fname = tools.get_output_filename('test.capsule') > > + self.assertTrue(os.path.exists(self.capsule_fname)) > > + > > + self._CheckCapsule(signed_capsule=True) > > + > > + def testCapsuleGenCfgFile(self): > > + """Test generation of EFI capsule through config file""" > > + self.payload_data = tools.to_bytes(TEXT_DATA) > > + > > + self._GenCapsuleCfgPayload('payload.txt', self.payload_data) > > + > > + self._DoReadFile('284_capsule_conf.dts') > > + > > + self.capsule_fname = '/tmp/capsules/test.capsule' > > + self.assertTrue(os.path.exists(self.capsule_fname)) > > + > > + self._CheckCapsule() > > + > > + def testCapsuleGenVersionSupport(self): > > + """Test generation of EFI capsule with version support""" > > + self.payload_data = tools.to_bytes(TEXT_DATA) > > > > + TestFunctional._MakeInputFile('payload.txt', self.payload_data) > > + > > + self._DoReadFile('290_capsule_version.dts') > > + > > + self.capsule_fname = tools.get_output_filename('test.capsule') > > + self.assertTrue(os.path.exists(self.capsule_fname)) > > + > > + self._CheckCapsule(version_check=True) > > + > > + def testCapsuleGenKeyMissing(self): > > + """Test that binman errors out on missing key""" > > + with self.assertRaises(ValueError) as e: > > + self._DoReadFile('285_capsule_missing_key.dts') > > + > > + self.assertIn("Both private-key and public key Certificate need to be provided", > > private key > certificate Okay > > > + str(e.exception)) > > + > > + def testCapsuleGenIndexMissing(self): > > + """Test that binman errors out on missing image index""" > > + with self.assertRaises(ValueError) as e: > > + self._DoReadFile('286_capsule_missing_index.dts') > > + > > + self.assertIn("mkeficapsule must be provided an 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('287_capsule_missing_guid.dts') > > + > > + self.assertIn("mkeficapsule must be provided an Image GUID", > > + str(e.exception)) > > + > > + def testCapsuleGenPayloadMissing(self): > > + """Test that binman errors out on missing input(payload)image""" > > + with self.assertRaises(ValueError) as e: > > + self._DoReadFile('288_capsule_missing_payload.dts') > > + > > + self.assertIn("mkeficapsule must be provided an input filename(payload)", > > + str(e.exception)) > > + > > + def testCapsuleGenCapsuleFileMissing(self): > > + """Test that binman errors out on missing output capsule file""" > > + with self.assertRaises(ValueError) as e: > > + self._DoReadFile('289_capsule_missing.dts') > > + > > + self.assertIn("Specify the output capsule file", > > + str(e.exception)) > > This looks good. It is a pain that you need to cover each missing arg. > I'm not sure I can think of a better way. > > > + > > if __name__ == "__main__": > > unittest.main() > > diff --git a/tools/binman/test/282_capsule.dts b/tools/binman/test/282_capsule.dts > > new file mode 100644 > > index 0000000000..0e7fcdf8ab > > --- /dev/null > > +++ b/tools/binman/test/282_capsule.dts > > @@ -0,0 +1,18 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > + > > +/dts-v1/; > > + > > +/ { > > + #address-cells = <1>; > > + #size-cells = <1>; > > + > > + binman { > > + capsule { > > + image-index = <0x1>; > > + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; > > Is there a #define somewhere for this? Perhaps you can add a #define, > or a comment as to what this is. I will put a comment. > > Also, please use lower case. Okay -sughosh > > > + hardware-instance = <0x0>; > > + filename = "payload.txt"; > > + capsule = "test.capsule"; > > + }; > > + }; > > +}; > > Regards, > Simon
Hi Sughosh, On Mon, 17 Jul 2023 at 04:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > hi Simon, > > On Sun, 16 Jul 2023 at 05:12, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Sat, 15 Jul 2023 at 07:46, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > Add support in binman for generating capsules. The capsule parameters > > > can be specified either through a config file or through the capsule > > > binman entry. Also add test cases in binman for capsule generation, > > > and enable this testing on the sandbox_spl variant. > > > > Can you use sandbox instead, or perhaps sandbox_spl? SPL is really for > > SPL testing. > > Er, I am actually using the sandbox_spl variant. > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > --- > > > Changes since V3: > > > * Add test cases for covering the various capsule generation > > > scenarios. > > > * Add function comments in the mkeficapsule bintool. > > > * Fix the fetch method of the mkeficapsule bintool to enable building > > > the tool. > > > * Add more details about the capsule parameters in the documentation > > > as well as the code. > > > * Fix order of module imports, and addition of blank lines in the > > > capsule.py file. > > > * Use SetContents in the ObtainContents method. > > > > > > configs/sandbox_spl_defconfig | 1 + > > > tools/binman/btool/mkeficapsule.py | 158 ++++++++++++++++++ > > > tools/binman/entries.rst | 37 ++++ > > > tools/binman/etype/capsule.py | 132 +++++++++++++++ > > > tools/binman/ftest.py | 127 ++++++++++++++ > > > tools/binman/test/282_capsule.dts | 18 ++ > > > tools/binman/test/283_capsule_signed.dts | 20 +++ > > > tools/binman/test/284_capsule_conf.dts | 14 ++ > > > tools/binman/test/285_capsule_missing_key.dts | 19 +++ > > > .../binman/test/286_capsule_missing_index.dts | 17 ++ > > > .../binman/test/287_capsule_missing_guid.dts | 17 ++ > > > .../test/288_capsule_missing_payload.dts | 17 ++ > > > tools/binman/test/289_capsule_missing.dts | 17 ++ > > > tools/binman/test/290_capsule_version.dts | 19 +++ > > > tools/binman/test/capsule_cfg.txt | 6 + > > > 15 files changed, 619 insertions(+) > > > create mode 100644 tools/binman/btool/mkeficapsule.py > > > create mode 100644 tools/binman/etype/capsule.py > > > create mode 100644 tools/binman/test/282_capsule.dts > > > create mode 100644 tools/binman/test/283_capsule_signed.dts > > > create mode 100644 tools/binman/test/284_capsule_conf.dts > > > create mode 100644 tools/binman/test/285_capsule_missing_key.dts > > > create mode 100644 tools/binman/test/286_capsule_missing_index.dts > > > create mode 100644 tools/binman/test/287_capsule_missing_guid.dts > > > create mode 100644 tools/binman/test/288_capsule_missing_payload.dts > > > create mode 100644 tools/binman/test/289_capsule_missing.dts > > > create mode 100644 tools/binman/test/290_capsule_version.dts > > > create mode 100644 tools/binman/test/capsule_cfg.txt > > > > This looks pretty good to me. Some nits below > > > > > > > > diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig > > > index dd848c57c6..2fcc789347 100644 > > > --- a/configs/sandbox_spl_defconfig > > > +++ b/configs/sandbox_spl_defconfig > > > @@ -248,3 +248,4 @@ CONFIG_UNIT_TEST=y > > > CONFIG_SPL_UNIT_TEST=y > > > CONFIG_UT_TIME=y > > > CONFIG_UT_DM=y > > > +CONFIG_TOOLS_MKEFICAPSULE=y > > > > Why enabling this here? I don't think it is needed in sandbox_spl, but > > in any case it should be in a different patch if needed. > > The binman tests run on the sandbox_spl variant. When running the > capsule generation tests, the mkeficapsule tool should be present on > the board variant no? Can we run this on the 'sandbox' variant instead? [..] > > > > > + > > > + def ReadNode(self): > > > + super().ReadNode() > > > + > > > + self.cfg_file = fdt_util.GetString(self._node, 'cfg-file') > > > + if not self.cfg_file: > > > + self.image_index = fdt_util.GetInt(self._node, 'image-index') > > > + if not self.image_index: > > > + self.Raise('mkeficapsule must be provided an Image Index') > > > + > > > + self.image_guid = fdt_util.GetString(self._node, 'image-type-id') > > > + if not self.image_guid: > > > + self.Raise('mkeficapsule must be provided an Image GUID') > > > > Use self.required_props = ['image-type-id', ...] in your __init__(). > > Then this is automatic > > I should have clarified this during the earlier version itself. So > these parameters are mandatory only when not using the config file. In > the scenario of generating the capsules through config files, all > these parameters are provided through the config file. Hence these > explicit checks. Hmm, I think we should consider having two different etypes, then. It seems in fact that your entry type is doing 2-3 different things? [..] > > What if some of the inputs are missing? Does this handle missing blobs? > > Any missing input parameters are checked earlier itself. > > > > + if self.cfg_file: > > > + return self.mkeficapsule.capsule_cfg_file(self.cfg_file) > > > + elif self.auth: > > > + return self.mkeficapsule.cmdline_auth_capsule(self.image_index, > > > + self.image_guid, > > > + self.hardware_instance, > > > + self.monotonic_count, > > > + self.private_key, > > > + self.pub_key_cert, > > > + self.payload, > > > + self.capsule_fname, > > > + self.fw_version) > > > + else: > > > + return self.mkeficapsule.cmdline_capsule(self.image_index, > > > + self.image_guid, > > > + self.hardware_instance, > > > + self.payload, > > > + self.capsule_fname, > > > + self.fw_version) Here is where I wonder whether you are putting too much in a single etype. Here there are three different cases. Should we have 3 etypes? [..] > > > > We really should not have GUIDs in the code...they are a mess. > > You want the UEFI capsule generation to happen through binman, and not > mention GUIDs. That ain't happening :) I just don't want them open-coded. They are meaningless gibberish that no one can understand. Use #define or some other way to give them a name. If I wrote: writel(0x09812374, 0x8723728) you would have the same comment. > > > > > > + # Image GUID specified in the DTS > > > + self.image_guid = "52cfd7092007104791d108469b7fe9c8" > > > + self.fmp_signature = "4d535331" > > > + self.fmp_size = "10" > > > + self.fmp_fw_version = "02" > > > > These should really be local vars, not members. > > Okay > > > > > > + > > > + self.capsule_data = tools.read_file(self.capsule_fname) > > > > Pass the data in here and then you don't need to read the file > > So the file needs to be read here since the actual capsule generation > tool(tools/mkeficapsule) does not return any capsule data. Instead, > the data gets written to the capsule file, and the tool just returns a > pass/fail status. Sure, but you can read that data in the caller to this functoin. > > > > > > + > > > + self.assertEqual(self.capsule_guid, self.capsule_data.hex()[:32]) > > > + self.assertEqual(self.image_guid, self.capsule_data.hex()[96:128]) > > > + > > > + if version_check: > > > + self.assertEqual(self.fmp_signature, self.capsule_data.hex()[184:192]) > > > + self.assertEqual(self.fmp_size, self.capsule_data.hex()[192:194]) > > > + self.assertEqual(self.fmp_fw_version, self.capsule_data.hex()[200:202]) > > > + > > > + if signed_capsule: > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[4770:4778]) > > > > Where do these integer offsets come from? Please add a comment > > So, these are simply offsets in the output capsule file, which get > impacted based on the contents being put in the capsule, like > presence/absence of optional headers. I don't think putting a comment > really wll add any value, because these offsets are variable. OK then please add a comment to that effect, as well as how to figure them out when things change. > > > > > > + elif version_check: > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[216:224]) > > > + else: > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[184:192]) > > > + > > > + def _GenCapsuleCfgPayload(self, fname, contents): > > > + capsule_dir = '/tmp/capsules/' > > > > You can't write to /tmp > > > > Please use self._indir for input files - see how other tests do it > > For all other tests, I am indeed using _indir and outdir. But for > generation of capsules through a config file, we need to specify the > location where the output capsule file will be written to. Which is > the reason for the /tmp/capsules/. We are using this directory for > collating all capsule related files. Well, sorry, you can't do that. I think you should provide a relative path, rather than absolute...that should solve the problem [..] > > > > Please can you use your own test data, like EFI_DATA ? Also if you > > declare it as a binary string you can drop the call. > > > > For example: > > > > EFI_DATA = b'efi' > > I don't know why text_data cannot be used, but I will add the EFI_DATA. Well firstly it is not a 'bytes' string. Secondly you may as well have your own as we have done with other etypes. > > > > > > + > > > + TestFunctional._MakeInputFile('payload.txt', self.payload_data) > > > + > > > + self._DoReadFile('282_capsule.dts') > > > > data = self... > > Please see above. We need to read the capsule file. This applies for > all the related comments about using the data = self._DoReadFile... That needs to be fixed, since the output file should be the capsule. Why would the output file be anything else?? [..] Regards, Simon
hi Simon, On Wed, 19 Jul 2023 at 06:41, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Mon, 17 Jul 2023 at 04:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > hi Simon, > > > > On Sun, 16 Jul 2023 at 05:12, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Sat, 15 Jul 2023 at 07:46, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > Add support in binman for generating capsules. The capsule parameters > > > > can be specified either through a config file or through the capsule > > > > binman entry. Also add test cases in binman for capsule generation, > > > > and enable this testing on the sandbox_spl variant. > > > > > > Can you use sandbox instead, or perhaps sandbox_spl? SPL is really for > > > SPL testing. > > > > Er, I am actually using the sandbox_spl variant. > > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > --- > > > > Changes since V3: > > > > * Add test cases for covering the various capsule generation > > > > scenarios. > > > > * Add function comments in the mkeficapsule bintool. > > > > * Fix the fetch method of the mkeficapsule bintool to enable building > > > > the tool. > > > > * Add more details about the capsule parameters in the documentation > > > > as well as the code. > > > > * Fix order of module imports, and addition of blank lines in the > > > > capsule.py file. > > > > * Use SetContents in the ObtainContents method. > > > > > > > > configs/sandbox_spl_defconfig | 1 + > > > > tools/binman/btool/mkeficapsule.py | 158 ++++++++++++++++++ > > > > tools/binman/entries.rst | 37 ++++ > > > > tools/binman/etype/capsule.py | 132 +++++++++++++++ > > > > tools/binman/ftest.py | 127 ++++++++++++++ > > > > tools/binman/test/282_capsule.dts | 18 ++ > > > > tools/binman/test/283_capsule_signed.dts | 20 +++ > > > > tools/binman/test/284_capsule_conf.dts | 14 ++ > > > > tools/binman/test/285_capsule_missing_key.dts | 19 +++ > > > > .../binman/test/286_capsule_missing_index.dts | 17 ++ > > > > .../binman/test/287_capsule_missing_guid.dts | 17 ++ > > > > .../test/288_capsule_missing_payload.dts | 17 ++ > > > > tools/binman/test/289_capsule_missing.dts | 17 ++ > > > > tools/binman/test/290_capsule_version.dts | 19 +++ > > > > tools/binman/test/capsule_cfg.txt | 6 + > > > > 15 files changed, 619 insertions(+) > > > > create mode 100644 tools/binman/btool/mkeficapsule.py > > > > create mode 100644 tools/binman/etype/capsule.py > > > > create mode 100644 tools/binman/test/282_capsule.dts > > > > create mode 100644 tools/binman/test/283_capsule_signed.dts > > > > create mode 100644 tools/binman/test/284_capsule_conf.dts > > > > create mode 100644 tools/binman/test/285_capsule_missing_key.dts > > > > create mode 100644 tools/binman/test/286_capsule_missing_index.dts > > > > create mode 100644 tools/binman/test/287_capsule_missing_guid.dts > > > > create mode 100644 tools/binman/test/288_capsule_missing_payload.dts > > > > create mode 100644 tools/binman/test/289_capsule_missing.dts > > > > create mode 100644 tools/binman/test/290_capsule_version.dts > > > > create mode 100644 tools/binman/test/capsule_cfg.txt > > > > > > This looks pretty good to me. Some nits below > > > > > > > > > > > diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig > > > > index dd848c57c6..2fcc789347 100644 > > > > --- a/configs/sandbox_spl_defconfig > > > > +++ b/configs/sandbox_spl_defconfig > > > > @@ -248,3 +248,4 @@ CONFIG_UNIT_TEST=y > > > > CONFIG_SPL_UNIT_TEST=y > > > > CONFIG_UT_TIME=y > > > > CONFIG_UT_DM=y > > > > +CONFIG_TOOLS_MKEFICAPSULE=y > > > > > > Why enabling this here? I don't think it is needed in sandbox_spl, but > > > in any case it should be in a different patch if needed. > > > > The binman tests run on the sandbox_spl variant. When running the > > capsule generation tests, the mkeficapsule tool should be present on > > the board variant no? > > Can we run this on the 'sandbox' variant instead? The capsule tests can be run on sandbox. But the change in the sandbox_spl_defconfig is for adding support for the capsule tests which are run as part of the binman test suite in CI. So it would be a question of whether the rest of the binman tests in CI can be run on the sandbox variant. Or are there any specific set of tests which need to be run on the sandbox_spl variant? > > [..] > > > > > > > > + > > > > + def ReadNode(self): > > > > + super().ReadNode() > > > > + > > > > + self.cfg_file = fdt_util.GetString(self._node, 'cfg-file') > > > > + if not self.cfg_file: > > > > + self.image_index = fdt_util.GetInt(self._node, 'image-index') > > > > + if not self.image_index: > > > > + self.Raise('mkeficapsule must be provided an Image Index') > > > > + > > > > + self.image_guid = fdt_util.GetString(self._node, 'image-type-id') > > > > + if not self.image_guid: > > > > + self.Raise('mkeficapsule must be provided an Image GUID') > > > > > > Use self.required_props = ['image-type-id', ...] in your __init__(). > > > Then this is automatic > > > > I should have clarified this during the earlier version itself. So > > these parameters are mandatory only when not using the config file. In > > the scenario of generating the capsules through config files, all > > these parameters are provided through the config file. Hence these > > explicit checks. > > Hmm, I think we should consider having two different etypes, then. It > seems in fact that your entry type is doing 2-3 different things? I am wondering if having two different etypes might get confusing for the users. > > [..] > > > > What if some of the inputs are missing? Does this handle missing blobs? > > > > Any missing input parameters are checked earlier itself. > > > > > > + if self.cfg_file: > > > > + return self.mkeficapsule.capsule_cfg_file(self.cfg_file) > > > > + elif self.auth: > > > > + return self.mkeficapsule.cmdline_auth_capsule(self.image_index, > > > > + self.image_guid, > > > > + self.hardware_instance, > > > > + self.monotonic_count, > > > > + self.private_key, > > > > + self.pub_key_cert, > > > > + self.payload, > > > > + self.capsule_fname, > > > > + self.fw_version) > > > > + else: > > > > + return self.mkeficapsule.cmdline_capsule(self.image_index, > > > > + self.image_guid, > > > > + self.hardware_instance, > > > > + self.payload, > > > > + self.capsule_fname, > > > > + self.fw_version) > > Here is where I wonder whether you are putting too much in a single > etype. Here there are three different cases. Should we have 3 etypes? Basically the result in all the three cases is the same -- generation of a capsule file. Just that the input parameters being passed for generation are slightly different. There can be multiple entry types for these, but like I mentioned above, would having multiple entry types not be confusing for the users? > > [..] > > > > > > > We really should not have GUIDs in the code...they are a mess. > > > > You want the UEFI capsule generation to happen through binman, and not > > mention GUIDs. That ain't happening :) > > I just don't want them open-coded. They are meaningless gibberish that > no one can understand. Use #define or some other way to give them a > name. Yes, I will move the GUIDS under a variable. > > If I wrote: > > writel(0x09812374, 0x8723728) > > you would have the same comment. > > > > > > > > > > + # Image GUID specified in the DTS > > > > + self.image_guid = "52cfd7092007104791d108469b7fe9c8" > > > > + self.fmp_signature = "4d535331" > > > > + self.fmp_size = "10" > > > > + self.fmp_fw_version = "02" > > > > > > These should really be local vars, not members. > > > > Okay > > > > > > > > > + > > > > + self.capsule_data = tools.read_file(self.capsule_fname) > > > > > > Pass the data in here and then you don't need to read the file > > > > So the file needs to be read here since the actual capsule generation > > tool(tools/mkeficapsule) does not return any capsule data. Instead, > > the data gets written to the capsule file, and the tool just returns a > > pass/fail status. > > Sure, but you can read that data in the caller to this functoin. Yes, the data can be read in the caller, but that would mean duplicating the read in four functions. Instead the file read is happening in one place where the data is being used. > > > > > > > > > > + > > > > + self.assertEqual(self.capsule_guid, self.capsule_data.hex()[:32]) > > > > + self.assertEqual(self.image_guid, self.capsule_data.hex()[96:128]) > > > > + > > > > + if version_check: > > > > + self.assertEqual(self.fmp_signature, self.capsule_data.hex()[184:192]) > > > > + self.assertEqual(self.fmp_size, self.capsule_data.hex()[192:194]) > > > > + self.assertEqual(self.fmp_fw_version, self.capsule_data.hex()[200:202]) > > > > + > > > > + if signed_capsule: > > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[4770:4778]) > > > > > > Where do these integer offsets come from? Please add a comment > > > > So, these are simply offsets in the output capsule file, which get > > impacted based on the contents being put in the capsule, like > > presence/absence of optional headers. I don't think putting a comment > > really wll add any value, because these offsets are variable. > > OK then please add a comment to that effect, as well as how to figure > them out when things change. Okay > > > > > > > > > > + elif version_check: > > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[216:224]) > > > > + else: > > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[184:192]) > > > > + > > > > + def _GenCapsuleCfgPayload(self, fname, contents): > > > > + capsule_dir = '/tmp/capsules/' > > > > > > You can't write to /tmp > > > > > > Please use self._indir for input files - see how other tests do it > > > > For all other tests, I am indeed using _indir and outdir. But for > > generation of capsules through a config file, we need to specify the > > location where the output capsule file will be written to. Which is > > the reason for the /tmp/capsules/. We are using this directory for > > collating all capsule related files. > > Well, sorry, you can't do that. I think you should provide a relative > path, rather than absolute...that should solve the problem Using absolute paths is only being done in the case of testing capsule generation through config file -- all the rest of the test cases are using relative paths for the input file and the capsule file. For the generation of capsules through the config file, the paths either need to be absolute, or relative to the directory from which the mkeficapsule tool is being invoked. And I believe that the binman test is creating temporary directories at runtime for input and output files. What is the issue you see in using the /tmp/capsules/ path for generation of capsules. I see directories being created with relevant files under /opt for other tests. If you would want the capsules directory to be in some other location, I can change that. > > [..] > > > > > > > Please can you use your own test data, like EFI_DATA ? Also if you > > > declare it as a binary string you can drop the call. > > > > > > For example: > > > > > > EFI_DATA = b'efi' > > > > I don't know why text_data cannot be used, but I will add the EFI_DATA. > > Well firstly it is not a 'bytes' string. Secondly you may as well have > your own as we have done with other etypes. Okay > > > > > > > > > > + > > > > + TestFunctional._MakeInputFile('payload.txt', self.payload_data) > > > > + > > > > + self._DoReadFile('282_capsule.dts') > > > > > > data = self... > > > > Please see above. We need to read the capsule file. This applies for > > all the related comments about using the data = self._DoReadFile... > > That needs to be fixed, since the output file should be the capsule. > Why would the output file be anything else?? The output file is indeed a capsule, but that is being generated by the capsule generation tool which gets called from the bintool. So the mkeficapsule tool that the bintool calls results in generation of the capsule file. In that way, this does not map directly to the concept of an image in binman. And I was wondering if I should introduce a new property like 'dummy-image', so that the resultant <image>.bin and <image>.map files are not created -- they really are not needed in the case of capsule generation. Would you be fine with an introduction of such a property? -sughosh
Hi Sughosh, On Wed, 19 Jul 2023 at 02:42, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > hi Simon, > > On Wed, 19 Jul 2023 at 06:41, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Mon, 17 Jul 2023 at 04:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > hi Simon, > > > > > > On Sun, 16 Jul 2023 at 05:12, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Sughosh, > > > > > > > > On Sat, 15 Jul 2023 at 07:46, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > Add support in binman for generating capsules. The capsule parameters > > > > > can be specified either through a config file or through the capsule > > > > > binman entry. Also add test cases in binman for capsule generation, > > > > > and enable this testing on the sandbox_spl variant. > > > > > > > > Can you use sandbox instead, or perhaps sandbox_spl? SPL is really for > > > > SPL testing. > > > > > > Er, I am actually using the sandbox_spl variant. > > > > > > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > --- > > > > > Changes since V3: > > > > > * Add test cases for covering the various capsule generation > > > > > scenarios. > > > > > * Add function comments in the mkeficapsule bintool. > > > > > * Fix the fetch method of the mkeficapsule bintool to enable building > > > > > the tool. > > > > > * Add more details about the capsule parameters in the documentation > > > > > as well as the code. > > > > > * Fix order of module imports, and addition of blank lines in the > > > > > capsule.py file. > > > > > * Use SetContents in the ObtainContents method. > > > > > > > > > > configs/sandbox_spl_defconfig | 1 + > > > > > tools/binman/btool/mkeficapsule.py | 158 ++++++++++++++++++ > > > > > tools/binman/entries.rst | 37 ++++ > > > > > tools/binman/etype/capsule.py | 132 +++++++++++++++ > > > > > tools/binman/ftest.py | 127 ++++++++++++++ > > > > > tools/binman/test/282_capsule.dts | 18 ++ > > > > > tools/binman/test/283_capsule_signed.dts | 20 +++ > > > > > tools/binman/test/284_capsule_conf.dts | 14 ++ > > > > > tools/binman/test/285_capsule_missing_key.dts | 19 +++ > > > > > .../binman/test/286_capsule_missing_index.dts | 17 ++ > > > > > .../binman/test/287_capsule_missing_guid.dts | 17 ++ > > > > > .../test/288_capsule_missing_payload.dts | 17 ++ > > > > > tools/binman/test/289_capsule_missing.dts | 17 ++ > > > > > tools/binman/test/290_capsule_version.dts | 19 +++ > > > > > tools/binman/test/capsule_cfg.txt | 6 + > > > > > 15 files changed, 619 insertions(+) > > > > > create mode 100644 tools/binman/btool/mkeficapsule.py > > > > > create mode 100644 tools/binman/etype/capsule.py > > > > > create mode 100644 tools/binman/test/282_capsule.dts > > > > > create mode 100644 tools/binman/test/283_capsule_signed.dts > > > > > create mode 100644 tools/binman/test/284_capsule_conf.dts > > > > > create mode 100644 tools/binman/test/285_capsule_missing_key.dts > > > > > create mode 100644 tools/binman/test/286_capsule_missing_index.dts > > > > > create mode 100644 tools/binman/test/287_capsule_missing_guid.dts > > > > > create mode 100644 tools/binman/test/288_capsule_missing_payload.dts > > > > > create mode 100644 tools/binman/test/289_capsule_missing.dts > > > > > create mode 100644 tools/binman/test/290_capsule_version.dts > > > > > create mode 100644 tools/binman/test/capsule_cfg.txt > > > > > > > > This looks pretty good to me. Some nits below > > > > > > > > > > > > > > diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig > > > > > index dd848c57c6..2fcc789347 100644 > > > > > --- a/configs/sandbox_spl_defconfig > > > > > +++ b/configs/sandbox_spl_defconfig > > > > > @@ -248,3 +248,4 @@ CONFIG_UNIT_TEST=y > > > > > CONFIG_SPL_UNIT_TEST=y > > > > > CONFIG_UT_TIME=y > > > > > CONFIG_UT_DM=y > > > > > +CONFIG_TOOLS_MKEFICAPSULE=y > > > > > > > > Why enabling this here? I don't think it is needed in sandbox_spl, but > > > > in any case it should be in a different patch if needed. > > > > > > The binman tests run on the sandbox_spl variant. When running the > > > capsule generation tests, the mkeficapsule tool should be present on > > > the board variant no? > > > > Can we run this on the 'sandbox' variant instead? > > The capsule tests can be run on sandbox. But the change in the > sandbox_spl_defconfig is for adding support for the capsule tests > which are run as part of the binman test suite in CI. So it would be a The binman tests are run separately, in 'Run binman, buildman, dtoc, Kconfig and patman testsuites' in gitlab. > question of whether the rest of the binman tests in CI can be run on > the sandbox variant. Or are there any specific set of tests which need > to be run on the sandbox_spl variant? Just the ones that need SPL. If you type 'make qcheck' you will see most/all of the tests, including the sandbox_spl ones. > > > > > [..] > > > > > > > > > > > + > > > > > + def ReadNode(self): > > > > > + super().ReadNode() > > > > > + > > > > > + self.cfg_file = fdt_util.GetString(self._node, 'cfg-file') > > > > > + if not self.cfg_file: > > > > > + self.image_index = fdt_util.GetInt(self._node, 'image-index') > > > > > + if not self.image_index: > > > > > + self.Raise('mkeficapsule must be provided an Image Index') > > > > > + > > > > > + self.image_guid = fdt_util.GetString(self._node, 'image-type-id') > > > > > + if not self.image_guid: > > > > > + self.Raise('mkeficapsule must be provided an Image GUID') > > > > > > > > Use self.required_props = ['image-type-id', ...] in your __init__(). > > > > Then this is automatic > > > > > > I should have clarified this during the earlier version itself. So > > > these parameters are mandatory only when not using the config file. In > > > the scenario of generating the capsules through config files, all > > > these parameters are provided through the config file. Hence these > > > explicit checks. > > > > Hmm, I think we should consider having two different etypes, then. It > > seems in fact that your entry type is doing 2-3 different things? > > I am wondering if having two different etypes might get confusing for the users. Maybe, but I'm already confused. > > > > > [..] > > > > > > What if some of the inputs are missing? Does this handle missing blobs? > > > > > > Any missing input parameters are checked earlier itself. > > > > > > > > + if self.cfg_file: > > > > > + return self.mkeficapsule.capsule_cfg_file(self.cfg_file) > > > > > + elif self.auth: > > > > > + return self.mkeficapsule.cmdline_auth_capsule(self.image_index, > > > > > + self.image_guid, > > > > > + self.hardware_instance, > > > > > + self.monotonic_count, > > > > > + self.private_key, > > > > > + self.pub_key_cert, > > > > > + self.payload, > > > > > + self.capsule_fname, > > > > > + self.fw_version) > > > > > + else: > > > > > + return self.mkeficapsule.cmdline_capsule(self.image_index, > > > > > + self.image_guid, > > > > > + self.hardware_instance, > > > > > + self.payload, > > > > > + self.capsule_fname, > > > > > + self.fw_version) > > > > Here is where I wonder whether you are putting too much in a single > > etype. Here there are three different cases. Should we have 3 etypes? > > Basically the result in all the three cases is the same -- generation > of a capsule file. Just that the input parameters being passed for > generation are slightly different. There can be multiple entry types > for these, but like I mentioned above, would having multiple entry > types not be confusing for the users? So can we drop the use of a cfg file? What is the difference between the second two? If you had added comments I would not have had to ask. > > > > > [..] > > > > > > > > > > We really should not have GUIDs in the code...they are a mess. > > > > > > You want the UEFI capsule generation to happen through binman, and not > > > mention GUIDs. That ain't happening :) > > > > I just don't want them open-coded. They are meaningless gibberish that > > no one can understand. Use #define or some other way to give them a > > name. > > Yes, I will move the GUIDS under a variable. > > > > > If I wrote: > > > > writel(0x09812374, 0x8723728) > > > > you would have the same comment. > > > > > > > > > > > > > > + # Image GUID specified in the DTS > > > > > + self.image_guid = "52cfd7092007104791d108469b7fe9c8" > > > > > + self.fmp_signature = "4d535331" > > > > > + self.fmp_size = "10" > > > > > + self.fmp_fw_version = "02" > > > > > > > > These should really be local vars, not members. > > > > > > Okay > > > > > > > > > > > > + > > > > > + self.capsule_data = tools.read_file(self.capsule_fname) > > > > > > > > Pass the data in here and then you don't need to read the file > > > > > > So the file needs to be read here since the actual capsule generation > > > tool(tools/mkeficapsule) does not return any capsule data. Instead, > > > the data gets written to the capsule file, and the tool just returns a > > > pass/fail status. > > > > Sure, but you can read that data in the caller to this functoin. > > Yes, the data can be read in the caller, but that would mean > duplicating the read in four functions. Instead the file read is > happening in one place where the data is being used. What do you mean? The data is returned from the call to self._DoReadFile(), the so pattern is: data = self._DoReadFile(...) You should be able to see that throughout ftest.py What exactly is the confusion here? > > > > > > > > > > > > > > > + > > > > > + self.assertEqual(self.capsule_guid, self.capsule_data.hex()[:32]) > > > > > + self.assertEqual(self.image_guid, self.capsule_data.hex()[96:128]) > > > > > + > > > > > + if version_check: > > > > > + self.assertEqual(self.fmp_signature, self.capsule_data.hex()[184:192]) > > > > > + self.assertEqual(self.fmp_size, self.capsule_data.hex()[192:194]) > > > > > + self.assertEqual(self.fmp_fw_version, self.capsule_data.hex()[200:202]) > > > > > + > > > > > + if signed_capsule: > > > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[4770:4778]) > > > > > > > > Where do these integer offsets come from? Please add a comment > > > > > > So, these are simply offsets in the output capsule file, which get > > > impacted based on the contents being put in the capsule, like > > > presence/absence of optional headers. I don't think putting a comment > > > really wll add any value, because these offsets are variable. > > > > OK then please add a comment to that effect, as well as how to figure > > them out when things change. > > Okay > > > > > > > > > > > > > > > + elif version_check: > > > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[216:224]) > > > > > + else: > > > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[184:192]) > > > > > + > > > > > + def _GenCapsuleCfgPayload(self, fname, contents): > > > > > + capsule_dir = '/tmp/capsules/' > > > > > > > > You can't write to /tmp > > > > > > > > Please use self._indir for input files - see how other tests do it > > > > > > For all other tests, I am indeed using _indir and outdir. But for > > > generation of capsules through a config file, we need to specify the > > > location where the output capsule file will be written to. Which is > > > the reason for the /tmp/capsules/. We are using this directory for > > > collating all capsule related files. > > > > Well, sorry, you can't do that. I think you should provide a relative > > path, rather than absolute...that should solve the problem > > Using absolute paths is only being done in the case of testing capsule > generation through config file -- all the rest of the test cases are > using relative paths for the input file and the capsule file. For the > generation of capsules through the config file, the paths either need > to be absolute, or relative to the directory from which the > mkeficapsule tool is being invoked. And I believe that the binman > test is creating temporary directories at runtime for input and output > files. OK so let's drop the cfg file stuff. It doesn't seem to be needed. > > What is the issue you see in using the /tmp/capsules/ path for > generation of capsules. I see directories being created with relevant > files under /opt for other tests. If you would want the capsules > directory to be in some other location, I can change that. Binman tests should use the input and output directories, so please figure out how to do this. If you drop the cfg file then it should be easy. > > > > > [..] > > > > > > > > > > Please can you use your own test data, like EFI_DATA ? Also if you > > > > declare it as a binary string you can drop the call. > > > > > > > > For example: > > > > > > > > EFI_DATA = b'efi' > > > > > > I don't know why text_data cannot be used, but I will add the EFI_DATA. > > > > Well firstly it is not a 'bytes' string. Secondly you may as well have > > your own as we have done with other etypes. > > Okay > > > > > > > > > > > > > > > + > > > > > + TestFunctional._MakeInputFile('payload.txt', self.payload_data) > > > > > + > > > > > + self._DoReadFile('282_capsule.dts') > > > > > > > > data = self... > > > > > > Please see above. We need to read the capsule file. This applies for > > > all the related comments about using the data = self._DoReadFile... > > > > That needs to be fixed, since the output file should be the capsule. > > Why would the output file be anything else?? > > The output file is indeed a capsule, but that is being generated by > the capsule generation tool which gets called from the bintool. So the > mkeficapsule tool that the bintool calls results in generation of the > capsule file. In that way, this does not map directly to the concept > of an image in binman. And I was wondering if I should introduce a new > property like 'dummy-image', so that the resultant <image>.bin and > <image>.map files are not created -- they really are not needed in the > case of capsule generation. Would you be fine with an introduction of > such a property? Eek, no. I thought the output file from binman (image.bin in most tests) was an EFI capsule. Is that not the case? If not, what exactly is it? Regards, Simon
hi Simon, On Thu, 20 Jul 2023 at 00:41, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Wed, 19 Jul 2023 at 02:42, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > hi Simon, > > > > On Wed, 19 Jul 2023 at 06:41, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Mon, 17 Jul 2023 at 04:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > hi Simon, > > > > > > > > On Sun, 16 Jul 2023 at 05:12, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > Hi Sughosh, > > > > > > > > > > On Sat, 15 Jul 2023 at 07:46, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > > > Add support in binman for generating capsules. The capsule parameters > > > > > > can be specified either through a config file or through the capsule > > > > > > binman entry. Also add test cases in binman for capsule generation, > > > > > > and enable this testing on the sandbox_spl variant. > > > > > > > > > > Can you use sandbox instead, or perhaps sandbox_spl? SPL is really for > > > > > SPL testing. > > > > > > > > Er, I am actually using the sandbox_spl variant. > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > > --- > > > > > > Changes since V3: > > > > > > * Add test cases for covering the various capsule generation > > > > > > scenarios. > > > > > > * Add function comments in the mkeficapsule bintool. > > > > > > * Fix the fetch method of the mkeficapsule bintool to enable building > > > > > > the tool. > > > > > > * Add more details about the capsule parameters in the documentation > > > > > > as well as the code. > > > > > > * Fix order of module imports, and addition of blank lines in the > > > > > > capsule.py file. > > > > > > * Use SetContents in the ObtainContents method. > > > > > > > > > > > > configs/sandbox_spl_defconfig | 1 + > > > > > > tools/binman/btool/mkeficapsule.py | 158 ++++++++++++++++++ > > > > > > tools/binman/entries.rst | 37 ++++ > > > > > > tools/binman/etype/capsule.py | 132 +++++++++++++++ > > > > > > tools/binman/ftest.py | 127 ++++++++++++++ > > > > > > tools/binman/test/282_capsule.dts | 18 ++ > > > > > > tools/binman/test/283_capsule_signed.dts | 20 +++ > > > > > > tools/binman/test/284_capsule_conf.dts | 14 ++ > > > > > > tools/binman/test/285_capsule_missing_key.dts | 19 +++ > > > > > > .../binman/test/286_capsule_missing_index.dts | 17 ++ > > > > > > .../binman/test/287_capsule_missing_guid.dts | 17 ++ > > > > > > .../test/288_capsule_missing_payload.dts | 17 ++ > > > > > > tools/binman/test/289_capsule_missing.dts | 17 ++ > > > > > > tools/binman/test/290_capsule_version.dts | 19 +++ > > > > > > tools/binman/test/capsule_cfg.txt | 6 + > > > > > > 15 files changed, 619 insertions(+) > > > > > > create mode 100644 tools/binman/btool/mkeficapsule.py > > > > > > create mode 100644 tools/binman/etype/capsule.py > > > > > > create mode 100644 tools/binman/test/282_capsule.dts > > > > > > create mode 100644 tools/binman/test/283_capsule_signed.dts > > > > > > create mode 100644 tools/binman/test/284_capsule_conf.dts > > > > > > create mode 100644 tools/binman/test/285_capsule_missing_key.dts > > > > > > create mode 100644 tools/binman/test/286_capsule_missing_index.dts > > > > > > create mode 100644 tools/binman/test/287_capsule_missing_guid.dts > > > > > > create mode 100644 tools/binman/test/288_capsule_missing_payload.dts > > > > > > create mode 100644 tools/binman/test/289_capsule_missing.dts > > > > > > create mode 100644 tools/binman/test/290_capsule_version.dts > > > > > > create mode 100644 tools/binman/test/capsule_cfg.txt > > > > > > > > > > This looks pretty good to me. Some nits below > > > > > > > > > > > > > > > > > diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig > > > > > > index dd848c57c6..2fcc789347 100644 > > > > > > --- a/configs/sandbox_spl_defconfig > > > > > > +++ b/configs/sandbox_spl_defconfig > > > > > > @@ -248,3 +248,4 @@ CONFIG_UNIT_TEST=y > > > > > > CONFIG_SPL_UNIT_TEST=y > > > > > > CONFIG_UT_TIME=y > > > > > > CONFIG_UT_DM=y > > > > > > +CONFIG_TOOLS_MKEFICAPSULE=y > > > > > > > > > > Why enabling this here? I don't think it is needed in sandbox_spl, but > > > > > in any case it should be in a different patch if needed. > > > > > > > > The binman tests run on the sandbox_spl variant. When running the > > > > capsule generation tests, the mkeficapsule tool should be present on > > > > the board variant no? > > > > > > Can we run this on the 'sandbox' variant instead? > > > > The capsule tests can be run on sandbox. But the change in the > > sandbox_spl_defconfig is for adding support for the capsule tests > > which are run as part of the binman test suite in CI. So it would be a > > The binman tests are run separately, in 'Run binman, buildman, dtoc, > Kconfig and patman testsuites' in gitlab. Yes, and that uses the sandbox_spl variant build for running the tests. Which is why I added building the mkeficapsule tool for the variant. > > > question of whether the rest of the binman tests in CI can be run on > > the sandbox variant. Or are there any specific set of tests which need > > to be run on the sandbox_spl variant? > > Just the ones that need SPL. If you type 'make qcheck' you will see > most/all of the tests, including the sandbox_spl ones. Can we not just build the mkeficapsule tool in the sandbox_spl variant. That keeps things simple. > > > > > > > > > [..] > > > > > > > > > > > > > > + > > > > > > + def ReadNode(self): > > > > > > + super().ReadNode() > > > > > > + > > > > > > + self.cfg_file = fdt_util.GetString(self._node, 'cfg-file') > > > > > > + if not self.cfg_file: > > > > > > + self.image_index = fdt_util.GetInt(self._node, 'image-index') > > > > > > + if not self.image_index: > > > > > > + self.Raise('mkeficapsule must be provided an Image Index') > > > > > > + > > > > > > + self.image_guid = fdt_util.GetString(self._node, 'image-type-id') > > > > > > + if not self.image_guid: > > > > > > + self.Raise('mkeficapsule must be provided an Image GUID') > > > > > > > > > > Use self.required_props = ['image-type-id', ...] in your __init__(). > > > > > Then this is automatic > > > > > > > > I should have clarified this during the earlier version itself. So > > > > these parameters are mandatory only when not using the config file. In > > > > the scenario of generating the capsules through config files, all > > > > these parameters are provided through the config file. Hence these > > > > explicit checks. > > > > > > Hmm, I think we should consider having two different etypes, then. It > > > seems in fact that your entry type is doing 2-3 different things? > > > > I am wondering if having two different etypes might get confusing for the users. > > Maybe, but I'm already confused. > > > > > > > > > [..] > > > > > > > > What if some of the inputs are missing? Does this handle missing blobs? > > > > > > > > Any missing input parameters are checked earlier itself. > > > > > > > > > > + if self.cfg_file: > > > > > > + return self.mkeficapsule.capsule_cfg_file(self.cfg_file) > > > > > > + elif self.auth: > > > > > > + return self.mkeficapsule.cmdline_auth_capsule(self.image_index, > > > > > > + self.image_guid, > > > > > > + self.hardware_instance, > > > > > > + self.monotonic_count, > > > > > > + self.private_key, > > > > > > + self.pub_key_cert, > > > > > > + self.payload, > > > > > > + self.capsule_fname, > > > > > > + self.fw_version) > > > > > > + else: > > > > > > + return self.mkeficapsule.cmdline_capsule(self.image_index, > > > > > > + self.image_guid, > > > > > > + self.hardware_instance, > > > > > > + self.payload, > > > > > > + self.capsule_fname, > > > > > > + self.fw_version) > > > > > > Here is where I wonder whether you are putting too much in a single > > > etype. Here there are three different cases. Should we have 3 etypes? > > > > Basically the result in all the three cases is the same -- generation > > of a capsule file. Just that the input parameters being passed for > > generation are slightly different. There can be multiple entry types > > for these, but like I mentioned above, would having multiple entry > > types not be confusing for the users? > > So can we drop the use of a cfg file? What is the difference between > the second two? If you had added comments I would not have had to ask. Using a config file to specify all the parameters needed for building capsule(s) is a good feature to have. It also results in generation of all the needed capsules through a single invocation of the mkeficapsule command, instead of multiple calls per capsule file. > > > > > > > > > [..] > > > > > > > > > > > > > We really should not have GUIDs in the code...they are a mess. > > > > > > > > You want the UEFI capsule generation to happen through binman, and not > > > > mention GUIDs. That ain't happening :) > > > > > > I just don't want them open-coded. They are meaningless gibberish that > > > no one can understand. Use #define or some other way to give them a > > > name. > > > > Yes, I will move the GUIDS under a variable. > > > > > > > > If I wrote: > > > > > > writel(0x09812374, 0x8723728) > > > > > > you would have the same comment. > > > > > > > > > > > > > > > > > > + # Image GUID specified in the DTS > > > > > > + self.image_guid = "52cfd7092007104791d108469b7fe9c8" > > > > > > + self.fmp_signature = "4d535331" > > > > > > + self.fmp_size = "10" > > > > > > + self.fmp_fw_version = "02" > > > > > > > > > > These should really be local vars, not members. > > > > > > > > Okay > > > > > > > > > > > > > > > + > > > > > > + self.capsule_data = tools.read_file(self.capsule_fname) > > > > > > > > > > Pass the data in here and then you don't need to read the file > > > > > > > > So the file needs to be read here since the actual capsule generation > > > > tool(tools/mkeficapsule) does not return any capsule data. Instead, > > > > the data gets written to the capsule file, and the tool just returns a > > > > pass/fail status. > > > > > > Sure, but you can read that data in the caller to this functoin. > > > > Yes, the data can be read in the caller, but that would mean > > duplicating the read in four functions. Instead the file read is > > happening in one place where the data is being used. > > What do you mean? The data is returned from the call to > self._DoReadFile(), the so pattern is: > > data = self._DoReadFile(...) > > You should be able to see that throughout ftest.py > > What exactly is the confusion here? No confusion. Again, this does not work when generating capsules through a config file. > > > > > > > > > > > > > > > > > > > > > + > > > > > > + self.assertEqual(self.capsule_guid, self.capsule_data.hex()[:32]) > > > > > > + self.assertEqual(self.image_guid, self.capsule_data.hex()[96:128]) > > > > > > + > > > > > > + if version_check: > > > > > > + self.assertEqual(self.fmp_signature, self.capsule_data.hex()[184:192]) > > > > > > + self.assertEqual(self.fmp_size, self.capsule_data.hex()[192:194]) > > > > > > + self.assertEqual(self.fmp_fw_version, self.capsule_data.hex()[200:202]) > > > > > > + > > > > > > + if signed_capsule: > > > > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[4770:4778]) > > > > > > > > > > Where do these integer offsets come from? Please add a comment > > > > > > > > So, these are simply offsets in the output capsule file, which get > > > > impacted based on the contents being put in the capsule, like > > > > presence/absence of optional headers. I don't think putting a comment > > > > really wll add any value, because these offsets are variable. > > > > > > OK then please add a comment to that effect, as well as how to figure > > > them out when things change. > > > > Okay > > > > > > > > > > > > > > > > > > > > + elif version_check: > > > > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[216:224]) > > > > > > + else: > > > > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[184:192]) > > > > > > + > > > > > > + def _GenCapsuleCfgPayload(self, fname, contents): > > > > > > + capsule_dir = '/tmp/capsules/' > > > > > > > > > > You can't write to /tmp > > > > > > > > > > Please use self._indir for input files - see how other tests do it > > > > > > > > For all other tests, I am indeed using _indir and outdir. But for > > > > generation of capsules through a config file, we need to specify the > > > > location where the output capsule file will be written to. Which is > > > > the reason for the /tmp/capsules/. We are using this directory for > > > > collating all capsule related files. > > > > > > Well, sorry, you can't do that. I think you should provide a relative > > > path, rather than absolute...that should solve the problem > > > > Using absolute paths is only being done in the case of testing capsule > > generation through config file -- all the rest of the test cases are > > using relative paths for the input file and the capsule file. For the > > generation of capsules through the config file, the paths either need > > to be absolute, or relative to the directory from which the > > mkeficapsule tool is being invoked. And I believe that the binman > > test is creating temporary directories at runtime for input and output > > files. > > OK so let's drop the cfg file stuff. It doesn't seem to be needed. Instead of dropping support for generating capsules through a cfg file in binman, I would say drop test coverage in binman for that case. We can test this feature, that is not an issue, but you insist on using the binman input and output directories which get created at runtime. We cannot test the capsule generation with a cfg file with that restriction. > > > > > What is the issue you see in using the /tmp/capsules/ path for > > generation of capsules. I see directories being created with relevant > > files under /opt for other tests. If you would want the capsules > > directory to be in some other location, I can change that. > > Binman tests should use the input and output directories, so please > figure out how to do this. If you drop the cfg file then it should be > easy. If I could do it, I would not spend this time discussing with you. Like I said above, if you insist on not using a known location for the input and output files, maybe we can drop the test for cfg file -- this functionality does get tested as part of the capsule update feature testing. I would argue that generating capsules using a cfg file is beneficial and be retained. > > > > > > > > > [..] > > > > > > > > > > > > > Please can you use your own test data, like EFI_DATA ? Also if you > > > > > declare it as a binary string you can drop the call. > > > > > > > > > > For example: > > > > > > > > > > EFI_DATA = b'efi' > > > > > > > > I don't know why text_data cannot be used, but I will add the EFI_DATA. > > > > > > Well firstly it is not a 'bytes' string. Secondly you may as well have > > > your own as we have done with other etypes. > > > > Okay > > > > > > > > > > > > > > > > > > > > + > > > > > > + TestFunctional._MakeInputFile('payload.txt', self.payload_data) > > > > > > + > > > > > > + self._DoReadFile('282_capsule.dts') > > > > > > > > > > data = self... > > > > > > > > Please see above. We need to read the capsule file. This applies for > > > > all the related comments about using the data = self._DoReadFile... > > > > > > That needs to be fixed, since the output file should be the capsule. > > > Why would the output file be anything else?? > > > > The output file is indeed a capsule, but that is being generated by > > the capsule generation tool which gets called from the bintool. So the > > mkeficapsule tool that the bintool calls results in generation of the > > capsule file. In that way, this does not map directly to the concept > > of an image in binman. And I was wondering if I should introduce a new > > property like 'dummy-image', so that the resultant <image>.bin and > > <image>.map files are not created -- they really are not needed in the > > case of capsule generation. Would you be fine with an introduction of > > such a property? > > Eek, no. > > I thought the output file from binman (image.bin in most tests) was an > EFI capsule. Is that not the case? If not, what exactly is it? In this case, it is not so. Which is why I proposed the above property. What happens is that the bintool calls the actual capsule generation tool, and that tool writes the capsule contents to a file which is provided to the tool as a parameter. Binman does not have to create the image.bin file in this case, since the mkeficapsule tool has already created the capsule file -- creating the image.bin and image.map is superfluous here. -sughosh
Hi Sughosh, On Thu, 20 Jul 2023 at 03:20, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > hi Simon, > > On Thu, 20 Jul 2023 at 00:41, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Wed, 19 Jul 2023 at 02:42, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > hi Simon, > > > > > > On Wed, 19 Jul 2023 at 06:41, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Sughosh, > > > > > > > > On Mon, 17 Jul 2023 at 04:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > hi Simon, > > > > > > > > > > On Sun, 16 Jul 2023 at 05:12, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > On Sat, 15 Jul 2023 at 07:46, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > > > > > Add support in binman for generating capsules. The capsule parameters > > > > > > > can be specified either through a config file or through the capsule > > > > > > > binman entry. Also add test cases in binman for capsule generation, > > > > > > > and enable this testing on the sandbox_spl variant. > > > > > > > > > > > > Can you use sandbox instead, or perhaps sandbox_spl? SPL is really for > > > > > > SPL testing. > > > > > > > > > > Er, I am actually using the sandbox_spl variant. > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > > > --- > > > > > > > Changes since V3: > > > > > > > * Add test cases for covering the various capsule generation > > > > > > > scenarios. > > > > > > > * Add function comments in the mkeficapsule bintool. > > > > > > > * Fix the fetch method of the mkeficapsule bintool to enable building > > > > > > > the tool. > > > > > > > * Add more details about the capsule parameters in the documentation > > > > > > > as well as the code. > > > > > > > * Fix order of module imports, and addition of blank lines in the > > > > > > > capsule.py file. > > > > > > > * Use SetContents in the ObtainContents method. > > > > > > > > > > > > > > configs/sandbox_spl_defconfig | 1 + > > > > > > > tools/binman/btool/mkeficapsule.py | 158 ++++++++++++++++++ > > > > > > > tools/binman/entries.rst | 37 ++++ > > > > > > > tools/binman/etype/capsule.py | 132 +++++++++++++++ > > > > > > > tools/binman/ftest.py | 127 ++++++++++++++ > > > > > > > tools/binman/test/282_capsule.dts | 18 ++ > > > > > > > tools/binman/test/283_capsule_signed.dts | 20 +++ > > > > > > > tools/binman/test/284_capsule_conf.dts | 14 ++ > > > > > > > tools/binman/test/285_capsule_missing_key.dts | 19 +++ > > > > > > > .../binman/test/286_capsule_missing_index.dts | 17 ++ > > > > > > > .../binman/test/287_capsule_missing_guid.dts | 17 ++ > > > > > > > .../test/288_capsule_missing_payload.dts | 17 ++ > > > > > > > tools/binman/test/289_capsule_missing.dts | 17 ++ > > > > > > > tools/binman/test/290_capsule_version.dts | 19 +++ > > > > > > > tools/binman/test/capsule_cfg.txt | 6 + > > > > > > > 15 files changed, 619 insertions(+) > > > > > > > create mode 100644 tools/binman/btool/mkeficapsule.py > > > > > > > create mode 100644 tools/binman/etype/capsule.py > > > > > > > create mode 100644 tools/binman/test/282_capsule.dts > > > > > > > create mode 100644 tools/binman/test/283_capsule_signed.dts > > > > > > > create mode 100644 tools/binman/test/284_capsule_conf.dts > > > > > > > create mode 100644 tools/binman/test/285_capsule_missing_key.dts > > > > > > > create mode 100644 tools/binman/test/286_capsule_missing_index.dts > > > > > > > create mode 100644 tools/binman/test/287_capsule_missing_guid.dts > > > > > > > create mode 100644 tools/binman/test/288_capsule_missing_payload.dts > > > > > > > create mode 100644 tools/binman/test/289_capsule_missing.dts > > > > > > > create mode 100644 tools/binman/test/290_capsule_version.dts > > > > > > > create mode 100644 tools/binman/test/capsule_cfg.txt > > > > > > > > > > > > This looks pretty good to me. Some nits below > > > > > > > > > > > > > > > > > > > > diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig > > > > > > > index dd848c57c6..2fcc789347 100644 > > > > > > > --- a/configs/sandbox_spl_defconfig > > > > > > > +++ b/configs/sandbox_spl_defconfig > > > > > > > @@ -248,3 +248,4 @@ CONFIG_UNIT_TEST=y > > > > > > > CONFIG_SPL_UNIT_TEST=y > > > > > > > CONFIG_UT_TIME=y > > > > > > > CONFIG_UT_DM=y > > > > > > > +CONFIG_TOOLS_MKEFICAPSULE=y > > > > > > > > > > > > Why enabling this here? I don't think it is needed in sandbox_spl, but > > > > > > in any case it should be in a different patch if needed. > > > > > > > > > > The binman tests run on the sandbox_spl variant. When running the > > > > > capsule generation tests, the mkeficapsule tool should be present on > > > > > the board variant no? > > > > > > > > Can we run this on the 'sandbox' variant instead? > > > > > > The capsule tests can be run on sandbox. But the change in the > > > sandbox_spl_defconfig is for adding support for the capsule tests > > > which are run as part of the binman test suite in CI. So it would be a > > > > The binman tests are run separately, in 'Run binman, buildman, dtoc, > > Kconfig and patman testsuites' in gitlab. > > Yes, and that uses the sandbox_spl variant build for running the > tests. Which is why I added building the mkeficapsule tool for the > variant. Not really. It uses sandbox_spl to obtain the pylibfdt library, that's all. The tests should use sandbox. By enabling CONFIG_BINMAN you will get pylibfdt anyway. > > > > > > question of whether the rest of the binman tests in CI can be run on > > > the sandbox variant. Or are there any specific set of tests which need > > > to be run on the sandbox_spl variant? > > > > Just the ones that need SPL. If you type 'make qcheck' you will see > > most/all of the tests, including the sandbox_spl ones. > > Can we not just build the mkeficapsule tool in the sandbox_spl > variant. That keeps things simple. Why is it simple? It is actually a pain, because the vast majority of tests only need sandbox. We use sandbox_spl for tests that do something in SPL and need that phase of U-Boot to perform some sort of test. So far as I can tell, the EFI capsule thing works entirely in U-Boot proper. > > > > > > > > > > > > > > [..] > > > > > > > > > > > > > > > > > + > > > > > > > + def ReadNode(self): > > > > > > > + super().ReadNode() > > > > > > > + > > > > > > > + self.cfg_file = fdt_util.GetString(self._node, 'cfg-file') > > > > > > > + if not self.cfg_file: > > > > > > > + self.image_index = fdt_util.GetInt(self._node, 'image-index') > > > > > > > + if not self.image_index: > > > > > > > + self.Raise('mkeficapsule must be provided an Image Index') > > > > > > > + > > > > > > > + self.image_guid = fdt_util.GetString(self._node, 'image-type-id') > > > > > > > + if not self.image_guid: > > > > > > > + self.Raise('mkeficapsule must be provided an Image GUID') > > > > > > > > > > > > Use self.required_props = ['image-type-id', ...] in your __init__(). > > > > > > Then this is automatic > > > > > > > > > > I should have clarified this during the earlier version itself. So > > > > > these parameters are mandatory only when not using the config file. In > > > > > the scenario of generating the capsules through config files, all > > > > > these parameters are provided through the config file. Hence these > > > > > explicit checks. > > > > > > > > Hmm, I think we should consider having two different etypes, then. It > > > > seems in fact that your entry type is doing 2-3 different things? > > > > > > I am wondering if having two different etypes might get confusing for the users. > > > > Maybe, but I'm already confused. > > > > > > > > > > > > > [..] > > > > > > > > > > What if some of the inputs are missing? Does this handle missing blobs? > > > > > > > > > > Any missing input parameters are checked earlier itself. > > > > > > > > > > > > + if self.cfg_file: > > > > > > > + return self.mkeficapsule.capsule_cfg_file(self.cfg_file) > > > > > > > + elif self.auth: > > > > > > > + return self.mkeficapsule.cmdline_auth_capsule(self.image_index, > > > > > > > + self.image_guid, > > > > > > > + self.hardware_instance, > > > > > > > + self.monotonic_count, > > > > > > > + self.private_key, > > > > > > > + self.pub_key_cert, > > > > > > > + self.payload, > > > > > > > + self.capsule_fname, > > > > > > > + self.fw_version) > > > > > > > + else: > > > > > > > + return self.mkeficapsule.cmdline_capsule(self.image_index, > > > > > > > + self.image_guid, > > > > > > > + self.hardware_instance, > > > > > > > + self.payload, > > > > > > > + self.capsule_fname, > > > > > > > + self.fw_version) > > > > > > > > Here is where I wonder whether you are putting too much in a single > > > > etype. Here there are three different cases. Should we have 3 etypes? > > > > > > Basically the result in all the three cases is the same -- generation > > > of a capsule file. Just that the input parameters being passed for > > > generation are slightly different. There can be multiple entry types > > > for these, but like I mentioned above, would having multiple entry > > > types not be confusing for the users? > > > > So can we drop the use of a cfg file? What is the difference between > > the second two? If you had added comments I would not have had to ask. > > Using a config file to specify all the parameters needed for building > capsule(s) is a good feature to have. It also results in generation of > all the needed capsules through a single invocation of the > mkeficapsule command, instead of multiple calls per capsule file. Does that matter? I don't really have an objection to it, except in that it makes the entry type confusing. Do we need to support this mode of operation in binman? Perhaps EFI capsule can do what ever other enrty type does and specify the params in the Binman node? You can still use the config file for other uses, but I don't see any use for it in binman. If you want multiple capsules, wouldn't that be done with multiple capsule nodes in the image definition? > > > > > > > > > > > > > > [..] > > > > > > > > > > > > > > > > We really should not have GUIDs in the code...they are a mess. > > > > > > > > > > You want the UEFI capsule generation to happen through binman, and not > > > > > mention GUIDs. That ain't happening :) > > > > > > > > I just don't want them open-coded. They are meaningless gibberish that > > > > no one can understand. Use #define or some other way to give them a > > > > name. > > > > > > Yes, I will move the GUIDS under a variable. > > > > > > > > > > > If I wrote: > > > > > > > > writel(0x09812374, 0x8723728) > > > > > > > > you would have the same comment. > > > > > > > > > > > > > > > > > > > > > > + # Image GUID specified in the DTS > > > > > > > + self.image_guid = "52cfd7092007104791d108469b7fe9c8" > > > > > > > + self.fmp_signature = "4d535331" > > > > > > > + self.fmp_size = "10" > > > > > > > + self.fmp_fw_version = "02" > > > > > > > > > > > > These should really be local vars, not members. > > > > > > > > > > Okay > > > > > > > > > > > > > > > > > > + > > > > > > > + self.capsule_data = tools.read_file(self.capsule_fname) > > > > > > > > > > > > Pass the data in here and then you don't need to read the file > > > > > > > > > > So the file needs to be read here since the actual capsule generation > > > > > tool(tools/mkeficapsule) does not return any capsule data. Instead, > > > > > the data gets written to the capsule file, and the tool just returns a > > > > > pass/fail status. > > > > > > > > Sure, but you can read that data in the caller to this functoin. > > > > > > Yes, the data can be read in the caller, but that would mean > > > duplicating the read in four functions. Instead the file read is > > > happening in one place where the data is being used. > > > > What do you mean? The data is returned from the call to > > self._DoReadFile(), the so pattern is: > > > > data = self._DoReadFile(...) > > > > You should be able to see that throughout ftest.py > > > > What exactly is the confusion here? > > No confusion. Again, this does not work when generating capsules > through a config file. OK let's drop the config file from binman. We don't need it and I'm tired of the back and forth. > > > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > + self.assertEqual(self.capsule_guid, self.capsule_data.hex()[:32]) > > > > > > > + self.assertEqual(self.image_guid, self.capsule_data.hex()[96:128]) > > > > > > > + > > > > > > > + if version_check: > > > > > > > + self.assertEqual(self.fmp_signature, self.capsule_data.hex()[184:192]) > > > > > > > + self.assertEqual(self.fmp_size, self.capsule_data.hex()[192:194]) > > > > > > > + self.assertEqual(self.fmp_fw_version, self.capsule_data.hex()[200:202]) > > > > > > > + > > > > > > > + if signed_capsule: > > > > > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[4770:4778]) > > > > > > > > > > > > Where do these integer offsets come from? Please add a comment > > > > > > > > > > So, these are simply offsets in the output capsule file, which get > > > > > impacted based on the contents being put in the capsule, like > > > > > presence/absence of optional headers. I don't think putting a comment > > > > > really wll add any value, because these offsets are variable. > > > > > > > > OK then please add a comment to that effect, as well as how to figure > > > > them out when things change. > > > > > > Okay > > > > > > > > > > > > > > > > > > > > > > > > > + elif version_check: > > > > > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[216:224]) > > > > > > > + else: > > > > > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[184:192]) > > > > > > > + > > > > > > > + def _GenCapsuleCfgPayload(self, fname, contents): > > > > > > > + capsule_dir = '/tmp/capsules/' > > > > > > > > > > > > You can't write to /tmp > > > > > > > > > > > > Please use self._indir for input files - see how other tests do it > > > > > > > > > > For all other tests, I am indeed using _indir and outdir. But for > > > > > generation of capsules through a config file, we need to specify the > > > > > location where the output capsule file will be written to. Which is > > > > > the reason for the /tmp/capsules/. We are using this directory for > > > > > collating all capsule related files. > > > > > > > > Well, sorry, you can't do that. I think you should provide a relative > > > > path, rather than absolute...that should solve the problem > > > > > > Using absolute paths is only being done in the case of testing capsule > > > generation through config file -- all the rest of the test cases are > > > using relative paths for the input file and the capsule file. For the > > > generation of capsules through the config file, the paths either need > > > to be absolute, or relative to the directory from which the > > > mkeficapsule tool is being invoked. And I believe that the binman > > > test is creating temporary directories at runtime for input and output > > > files. > > > > OK so let's drop the cfg file stuff. It doesn't seem to be needed. > > Instead of dropping support for generating capsules through a cfg file > in binman, I would say drop test coverage in binman for that case. We > can test this feature, that is not an issue, but you insist on using > the binman input and output directories which get created at runtime. > We cannot test the capsule generation with a cfg file with that > restriction. No. > > > > > > > > > > What is the issue you see in using the /tmp/capsules/ path for > > > generation of capsules. I see directories being created with relevant > > > files under /opt for other tests. If you would want the capsules > > > directory to be in some other location, I can change that. > > > > Binman tests should use the input and output directories, so please > > figure out how to do this. If you drop the cfg file then it should be > > easy. > > If I could do it, I would not spend this time discussing with you. > Like I said above, if you insist on not using a known location for the > input and output files, maybe we can drop the test for cfg file -- > this functionality does get tested as part of the capsule update > feature testing. I would argue that generating capsules using a cfg > file is beneficial and be retained. As above. > > > > > > > > > > > > > > [..] > > > > > > > > > > > > > > > > Please can you use your own test data, like EFI_DATA ? Also if you > > > > > > declare it as a binary string you can drop the call. > > > > > > > > > > > > For example: > > > > > > > > > > > > EFI_DATA = b'efi' > > > > > > > > > > I don't know why text_data cannot be used, but I will add the EFI_DATA. > > > > > > > > Well firstly it is not a 'bytes' string. Secondly you may as well have > > > > your own as we have done with other etypes. > > > > > > Okay > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > + TestFunctional._MakeInputFile('payload.txt', self.payload_data) > > > > > > > + > > > > > > > + self._DoReadFile('282_capsule.dts') > > > > > > > > > > > > data = self... > > > > > > > > > > Please see above. We need to read the capsule file. This applies for > > > > > all the related comments about using the data = self._DoReadFile... > > > > > > > > That needs to be fixed, since the output file should be the capsule. > > > > Why would the output file be anything else?? > > > > > > The output file is indeed a capsule, but that is being generated by > > > the capsule generation tool which gets called from the bintool. So the > > > mkeficapsule tool that the bintool calls results in generation of the > > > capsule file. In that way, this does not map directly to the concept > > > of an image in binman. And I was wondering if I should introduce a new > > > property like 'dummy-image', so that the resultant <image>.bin and > > > <image>.map files are not created -- they really are not needed in the > > > case of capsule generation. Would you be fine with an introduction of > > > such a property? > > > > Eek, no. > > > > I thought the output file from binman (image.bin in most tests) was an > > EFI capsule. Is that not the case? If not, what exactly is it? > > In this case, it is not so. Which is why I proposed the above > property. What happens is that the bintool calls the actual capsule > generation tool, and that tool writes the capsule contents to a file > which is provided to the tool as a parameter. Binman does not have to > create the image.bin file in this case, since the mkeficapsule tool > has already created the capsule file -- creating the image.bin and > image.map is superfluous here. I think you are missing something here. The tools that binman uses are there to generate things that the binman description wants. The EFI capsule is just another one of those. We should rename 'capsule' to 'efi-capsule', I think, since there might be other types of capsule. In theory I could say: binman { u-boot-spl { }; efi-capsule { // things we want in the capsule u-boot { } } and it should produce an image with SPL at the start followed by the capsule. The capsule is allowed to generate data, not create the whole image. Perhaps that is the blocker here? I see now that your capsule entry type is wonky. It needs to accept properties (not filenames!) and use those to package up the contents, which should be subnodes of itself. Probably you should look at how mkimage does it...but you essentially call self.collect_contents_to_file() to pick up the data. Your capsule should probably subclass entry_section, rather than Entry, since entry_Section handles some other things for you, like missing blobs. Looking at your last patch I think this is the confusion, and why this has been so hard for me to wrap my head around. Regards, Simon
hi Simon, On Wed, 26 Jul 2023 at 04:06, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Thu, 20 Jul 2023 at 03:20, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > hi Simon, > > > > On Thu, 20 Jul 2023 at 00:41, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Wed, 19 Jul 2023 at 02:42, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > hi Simon, > > > > > > > > On Wed, 19 Jul 2023 at 06:41, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > Hi Sughosh, > > > > > > > > > > On Mon, 17 Jul 2023 at 04:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > > > hi Simon, > > > > > > > > > > > > On Sun, 16 Jul 2023 at 05:12, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > > > On Sat, 15 Jul 2023 at 07:46, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > > > > > > > Add support in binman for generating capsules. The capsule parameters > > > > > > > > can be specified either through a config file or through the capsule > > > > > > > > binman entry. Also add test cases in binman for capsule generation, > > > > > > > > and enable this testing on the sandbox_spl variant. > > > > > > > > > > > > > > Can you use sandbox instead, or perhaps sandbox_spl? SPL is really for > > > > > > > SPL testing. > > > > > > > > > > > > Er, I am actually using the sandbox_spl variant. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > > > > --- > > > > > > > > Changes since V3: > > > > > > > > * Add test cases for covering the various capsule generation > > > > > > > > scenarios. > > > > > > > > * Add function comments in the mkeficapsule bintool. > > > > > > > > * Fix the fetch method of the mkeficapsule bintool to enable building > > > > > > > > the tool. > > > > > > > > * Add more details about the capsule parameters in the documentation > > > > > > > > as well as the code. > > > > > > > > * Fix order of module imports, and addition of blank lines in the > > > > > > > > capsule.py file. > > > > > > > > * Use SetContents in the ObtainContents method. > > > > > > > > > > > > > > > > configs/sandbox_spl_defconfig | 1 + > > > > > > > > tools/binman/btool/mkeficapsule.py | 158 ++++++++++++++++++ > > > > > > > > tools/binman/entries.rst | 37 ++++ > > > > > > > > tools/binman/etype/capsule.py | 132 +++++++++++++++ > > > > > > > > tools/binman/ftest.py | 127 ++++++++++++++ > > > > > > > > tools/binman/test/282_capsule.dts | 18 ++ > > > > > > > > tools/binman/test/283_capsule_signed.dts | 20 +++ > > > > > > > > tools/binman/test/284_capsule_conf.dts | 14 ++ > > > > > > > > tools/binman/test/285_capsule_missing_key.dts | 19 +++ > > > > > > > > .../binman/test/286_capsule_missing_index.dts | 17 ++ > > > > > > > > .../binman/test/287_capsule_missing_guid.dts | 17 ++ > > > > > > > > .../test/288_capsule_missing_payload.dts | 17 ++ > > > > > > > > tools/binman/test/289_capsule_missing.dts | 17 ++ > > > > > > > > tools/binman/test/290_capsule_version.dts | 19 +++ > > > > > > > > tools/binman/test/capsule_cfg.txt | 6 + > > > > > > > > 15 files changed, 619 insertions(+) > > > > > > > > create mode 100644 tools/binman/btool/mkeficapsule.py > > > > > > > > create mode 100644 tools/binman/etype/capsule.py > > > > > > > > create mode 100644 tools/binman/test/282_capsule.dts > > > > > > > > create mode 100644 tools/binman/test/283_capsule_signed.dts > > > > > > > > create mode 100644 tools/binman/test/284_capsule_conf.dts > > > > > > > > create mode 100644 tools/binman/test/285_capsule_missing_key.dts > > > > > > > > create mode 100644 tools/binman/test/286_capsule_missing_index.dts > > > > > > > > create mode 100644 tools/binman/test/287_capsule_missing_guid.dts > > > > > > > > create mode 100644 tools/binman/test/288_capsule_missing_payload.dts > > > > > > > > create mode 100644 tools/binman/test/289_capsule_missing.dts > > > > > > > > create mode 100644 tools/binman/test/290_capsule_version.dts > > > > > > > > create mode 100644 tools/binman/test/capsule_cfg.txt > > > > > > > > > > > > > > This looks pretty good to me. Some nits below > > > > > > > > > > > > > > > > > > > > > > > diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig > > > > > > > > index dd848c57c6..2fcc789347 100644 > > > > > > > > --- a/configs/sandbox_spl_defconfig > > > > > > > > +++ b/configs/sandbox_spl_defconfig > > > > > > > > @@ -248,3 +248,4 @@ CONFIG_UNIT_TEST=y > > > > > > > > CONFIG_SPL_UNIT_TEST=y > > > > > > > > CONFIG_UT_TIME=y > > > > > > > > CONFIG_UT_DM=y > > > > > > > > +CONFIG_TOOLS_MKEFICAPSULE=y > > > > > > > > > > > > > > Why enabling this here? I don't think it is needed in sandbox_spl, but > > > > > > > in any case it should be in a different patch if needed. > > > > > > > > > > > > The binman tests run on the sandbox_spl variant. When running the > > > > > > capsule generation tests, the mkeficapsule tool should be present on > > > > > > the board variant no? > > > > > > > > > > Can we run this on the 'sandbox' variant instead? > > > > > > > > The capsule tests can be run on sandbox. But the change in the > > > > sandbox_spl_defconfig is for adding support for the capsule tests > > > > which are run as part of the binman test suite in CI. So it would be a > > > > > > The binman tests are run separately, in 'Run binman, buildman, dtoc, > > > Kconfig and patman testsuites' in gitlab. > > > > Yes, and that uses the sandbox_spl variant build for running the > > tests. Which is why I added building the mkeficapsule tool for the > > variant. > > Not really. It uses sandbox_spl to obtain the pylibfdt library, that's all. > > The tests should use sandbox. By enabling CONFIG_BINMAN you will get > pylibfdt anyway. I will check on this point. > > > > > > > > > > question of whether the rest of the binman tests in CI can be run on > > > > the sandbox variant. Or are there any specific set of tests which need > > > > to be run on the sandbox_spl variant? > > > > > > Just the ones that need SPL. If you type 'make qcheck' you will see > > > most/all of the tests, including the sandbox_spl ones. > > > > Can we not just build the mkeficapsule tool in the sandbox_spl > > variant. That keeps things simple. > > Why is it simple? It is actually a pain, because the vast majority of > tests only need sandbox. We use sandbox_spl for tests that do > something in SPL and need that phase of U-Boot to perform some sort of > test. So far as I can tell, the EFI capsule thing works entirely in > U-Boot proper. > > > > > > > > > > > > > > > > > > > > [..] > > > > > > > > > > > > > > > > > > > > + > > > > > > > > + def ReadNode(self): > > > > > > > > + super().ReadNode() > > > > > > > > + > > > > > > > > + self.cfg_file = fdt_util.GetString(self._node, 'cfg-file') > > > > > > > > + if not self.cfg_file: > > > > > > > > + self.image_index = fdt_util.GetInt(self._node, 'image-index') > > > > > > > > + if not self.image_index: > > > > > > > > + self.Raise('mkeficapsule must be provided an Image Index') > > > > > > > > + > > > > > > > > + self.image_guid = fdt_util.GetString(self._node, 'image-type-id') > > > > > > > > + if not self.image_guid: > > > > > > > > + self.Raise('mkeficapsule must be provided an Image GUID') > > > > > > > > > > > > > > Use self.required_props = ['image-type-id', ...] in your __init__(). > > > > > > > Then this is automatic > > > > > > > > > > > > I should have clarified this during the earlier version itself. So > > > > > > these parameters are mandatory only when not using the config file. In > > > > > > the scenario of generating the capsules through config files, all > > > > > > these parameters are provided through the config file. Hence these > > > > > > explicit checks. > > > > > > > > > > Hmm, I think we should consider having two different etypes, then. It > > > > > seems in fact that your entry type is doing 2-3 different things? > > > > > > > > I am wondering if having two different etypes might get confusing for the users. > > > > > > Maybe, but I'm already confused. > > > > > > > > > > > > > > > > > [..] > > > > > > > > > > > > What if some of the inputs are missing? Does this handle missing blobs? > > > > > > > > > > > > Any missing input parameters are checked earlier itself. > > > > > > > > > > > > > > + if self.cfg_file: > > > > > > > > + return self.mkeficapsule.capsule_cfg_file(self.cfg_file) > > > > > > > > + elif self.auth: > > > > > > > > + return self.mkeficapsule.cmdline_auth_capsule(self.image_index, > > > > > > > > + self.image_guid, > > > > > > > > + self.hardware_instance, > > > > > > > > + self.monotonic_count, > > > > > > > > + self.private_key, > > > > > > > > + self.pub_key_cert, > > > > > > > > + self.payload, > > > > > > > > + self.capsule_fname, > > > > > > > > + self.fw_version) > > > > > > > > + else: > > > > > > > > + return self.mkeficapsule.cmdline_capsule(self.image_index, > > > > > > > > + self.image_guid, > > > > > > > > + self.hardware_instance, > > > > > > > > + self.payload, > > > > > > > > + self.capsule_fname, > > > > > > > > + self.fw_version) > > > > > > > > > > Here is where I wonder whether you are putting too much in a single > > > > > etype. Here there are three different cases. Should we have 3 etypes? > > > > > > > > Basically the result in all the three cases is the same -- generation > > > > of a capsule file. Just that the input parameters being passed for > > > > generation are slightly different. There can be multiple entry types > > > > for these, but like I mentioned above, would having multiple entry > > > > types not be confusing for the users? > > > > > > So can we drop the use of a cfg file? What is the difference between > > > the second two? If you had added comments I would not have had to ask. > > > > Using a config file to specify all the parameters needed for building > > capsule(s) is a good feature to have. It also results in generation of > > all the needed capsules through a single invocation of the > > mkeficapsule command, instead of multiple calls per capsule file. > > Does that matter? I don't really have an objection to it, except in > that it makes the entry type confusing. Do we need to support this > mode of operation in binman? Perhaps EFI capsule can do what ever > other enrty type does and specify the params in the Binman node? You > can still use the config file for other uses, but I don't see any use > for it in binman. > > If you want multiple capsules, wouldn't that be done with multiple > capsule nodes in the image definition? > > > > > > > > > > > > > > > > > > > > [..] > > > > > > > > > > > > > > > > > > > We really should not have GUIDs in the code...they are a mess. > > > > > > > > > > > > You want the UEFI capsule generation to happen through binman, and not > > > > > > mention GUIDs. That ain't happening :) > > > > > > > > > > I just don't want them open-coded. They are meaningless gibberish that > > > > > no one can understand. Use #define or some other way to give them a > > > > > name. > > > > > > > > Yes, I will move the GUIDS under a variable. > > > > > > > > > > > > > > If I wrote: > > > > > > > > > > writel(0x09812374, 0x8723728) > > > > > > > > > > you would have the same comment. > > > > > > > > > > > > > > > > > > > > > > > > > > + # Image GUID specified in the DTS > > > > > > > > + self.image_guid = "52cfd7092007104791d108469b7fe9c8" > > > > > > > > + self.fmp_signature = "4d535331" > > > > > > > > + self.fmp_size = "10" > > > > > > > > + self.fmp_fw_version = "02" > > > > > > > > > > > > > > These should really be local vars, not members. > > > > > > > > > > > > Okay > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > + self.capsule_data = tools.read_file(self.capsule_fname) > > > > > > > > > > > > > > Pass the data in here and then you don't need to read the file > > > > > > > > > > > > So the file needs to be read here since the actual capsule generation > > > > > > tool(tools/mkeficapsule) does not return any capsule data. Instead, > > > > > > the data gets written to the capsule file, and the tool just returns a > > > > > > pass/fail status. > > > > > > > > > > Sure, but you can read that data in the caller to this functoin. > > > > > > > > Yes, the data can be read in the caller, but that would mean > > > > duplicating the read in four functions. Instead the file read is > > > > happening in one place where the data is being used. > > > > > > What do you mean? The data is returned from the call to > > > self._DoReadFile(), the so pattern is: > > > > > > data = self._DoReadFile(...) > > > > > > You should be able to see that throughout ftest.py > > > > > > What exactly is the confusion here? > > > > No confusion. Again, this does not work when generating capsules > > through a config file. > > OK let's drop the config file from binman. We don't need it and I'm > tired of the back and forth. Sorry about that, but like I mentioned in my other reply, we would be needing support for generating capsules through the config file. Can we not work around the testing in binman for this feature. Like I mentioned earlier, this will be getting tested as part of the larger efi capsule update feature testing. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > + self.assertEqual(self.capsule_guid, self.capsule_data.hex()[:32]) > > > > > > > > + self.assertEqual(self.image_guid, self.capsule_data.hex()[96:128]) > > > > > > > > + > > > > > > > > + if version_check: > > > > > > > > + self.assertEqual(self.fmp_signature, self.capsule_data.hex()[184:192]) > > > > > > > > + self.assertEqual(self.fmp_size, self.capsule_data.hex()[192:194]) > > > > > > > > + self.assertEqual(self.fmp_fw_version, self.capsule_data.hex()[200:202]) > > > > > > > > + > > > > > > > > + if signed_capsule: > > > > > > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[4770:4778]) > > > > > > > > > > > > > > Where do these integer offsets come from? Please add a comment > > > > > > > > > > > > So, these are simply offsets in the output capsule file, which get > > > > > > impacted based on the contents being put in the capsule, like > > > > > > presence/absence of optional headers. I don't think putting a comment > > > > > > really wll add any value, because these offsets are variable. > > > > > > > > > > OK then please add a comment to that effect, as well as how to figure > > > > > them out when things change. > > > > > > > > Okay > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + elif version_check: > > > > > > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[216:224]) > > > > > > > > + else: > > > > > > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[184:192]) > > > > > > > > + > > > > > > > > + def _GenCapsuleCfgPayload(self, fname, contents): > > > > > > > > + capsule_dir = '/tmp/capsules/' > > > > > > > > > > > > > > You can't write to /tmp > > > > > > > > > > > > > > Please use self._indir for input files - see how other tests do it > > > > > > > > > > > > For all other tests, I am indeed using _indir and outdir. But for > > > > > > generation of capsules through a config file, we need to specify the > > > > > > location where the output capsule file will be written to. Which is > > > > > > the reason for the /tmp/capsules/. We are using this directory for > > > > > > collating all capsule related files. > > > > > > > > > > Well, sorry, you can't do that. I think you should provide a relative > > > > > path, rather than absolute...that should solve the problem > > > > > > > > Using absolute paths is only being done in the case of testing capsule > > > > generation through config file -- all the rest of the test cases are > > > > using relative paths for the input file and the capsule file. For the > > > > generation of capsules through the config file, the paths either need > > > > to be absolute, or relative to the directory from which the > > > > mkeficapsule tool is being invoked. And I believe that the binman > > > > test is creating temporary directories at runtime for input and output > > > > files. > > > > > > OK so let's drop the cfg file stuff. It doesn't seem to be needed. > > > > Instead of dropping support for generating capsules through a cfg file > > in binman, I would say drop test coverage in binman for that case. We > > can test this feature, that is not an issue, but you insist on using > > the binman input and output directories which get created at runtime. > > We cannot test the capsule generation with a cfg file with that > > restriction. > > No. > > > > > > > > > > > > > > > > What is the issue you see in using the /tmp/capsules/ path for > > > > generation of capsules. I see directories being created with relevant > > > > files under /opt for other tests. If you would want the capsules > > > > directory to be in some other location, I can change that. > > > > > > Binman tests should use the input and output directories, so please > > > figure out how to do this. If you drop the cfg file then it should be > > > easy. > > > > If I could do it, I would not spend this time discussing with you. > > Like I said above, if you insist on not using a known location for the > > input and output files, maybe we can drop the test for cfg file -- > > this functionality does get tested as part of the capsule update > > feature testing. I would argue that generating capsules using a cfg > > file is beneficial and be retained. > > As above. > > > > > > > > > > > > > > > > > > > > [..] > > > > > > > > > > > > > > > > > > > Please can you use your own test data, like EFI_DATA ? Also if you > > > > > > > declare it as a binary string you can drop the call. > > > > > > > > > > > > > > For example: > > > > > > > > > > > > > > EFI_DATA = b'efi' > > > > > > > > > > > > I don't know why text_data cannot be used, but I will add the EFI_DATA. > > > > > > > > > > Well firstly it is not a 'bytes' string. Secondly you may as well have > > > > > your own as we have done with other etypes. > > > > > > > > Okay > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > + TestFunctional._MakeInputFile('payload.txt', self.payload_data) > > > > > > > > + > > > > > > > > + self._DoReadFile('282_capsule.dts') > > > > > > > > > > > > > > data = self... > > > > > > > > > > > > Please see above. We need to read the capsule file. This applies for > > > > > > all the related comments about using the data = self._DoReadFile... > > > > > > > > > > That needs to be fixed, since the output file should be the capsule. > > > > > Why would the output file be anything else?? > > > > > > > > The output file is indeed a capsule, but that is being generated by > > > > the capsule generation tool which gets called from the bintool. So the > > > > mkeficapsule tool that the bintool calls results in generation of the > > > > capsule file. In that way, this does not map directly to the concept > > > > of an image in binman. And I was wondering if I should introduce a new > > > > property like 'dummy-image', so that the resultant <image>.bin and > > > > <image>.map files are not created -- they really are not needed in the > > > > case of capsule generation. Would you be fine with an introduction of > > > > such a property? > > > > > > Eek, no. > > > > > > I thought the output file from binman (image.bin in most tests) was an > > > EFI capsule. Is that not the case? If not, what exactly is it? > > > > In this case, it is not so. Which is why I proposed the above > > property. What happens is that the bintool calls the actual capsule > > generation tool, and that tool writes the capsule contents to a file > > which is provided to the tool as a parameter. Binman does not have to > > create the image.bin file in this case, since the mkeficapsule tool > > has already created the capsule file -- creating the image.bin and > > image.map is superfluous here. > > I think you are missing something here. The tools that binman uses are > there to generate things that the binman description wants. The EFI > capsule is just another one of those. > > We should rename 'capsule' to 'efi-capsule', I think, since there > might be other types of capsule. > > In theory I could say: > > binman { > u-boot-spl { > }; > efi-capsule { > // things we want in the capsule > u-boot { > } > } > > and it should produce an image with SPL at the start followed by the > capsule. The capsule is allowed to generate data, not create the whole > image. Perhaps that is the blocker here? > > I see now that your capsule entry type is wonky. It needs to accept > properties (not filenames!) and use those to package up the contents, > which should be subnodes of itself. Okay. I will take a look at how mkimage is called. I had checked the intel_ifwi entry type for reference, and I did see use of the filename property there. > > Probably you should look at how mkimage does it...but you essentially > call self.collect_contents_to_file() to pick up the data. Your capsule > should probably subclass entry_section, rather than Entry, since > entry_Section handles some other things for you, like missing blobs. Okay, I will check. -sughosh > > Looking at your last patch I think this is the confusion, and why this > has been so hard for me to wrap my head around. > > Regards, > Simon
Hi Sughosh, On Wed, 26 Jul 2023 at 03:36, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > hi Simon, > > On Wed, 26 Jul 2023 at 04:06, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Thu, 20 Jul 2023 at 03:20, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > hi Simon, > > > > > > On Thu, 20 Jul 2023 at 00:41, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Sughosh, > > > > > > > > On Wed, 19 Jul 2023 at 02:42, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > hi Simon, > > > > > > > > > > On Wed, 19 Jul 2023 at 06:41, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > On Mon, 17 Jul 2023 at 04:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > > > > > hi Simon, > > > > > > > > > > > > > > On Sun, 16 Jul 2023 at 05:12, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > > > > > On Sat, 15 Jul 2023 at 07:46, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > > > > > > > > > Add support in binman for generating capsules. The capsule parameters > > > > > > > > > can be specified either through a config file or through the capsule > > > > > > > > > binman entry. Also add test cases in binman for capsule generation, > > > > > > > > > and enable this testing on the sandbox_spl variant. > > > > > > > > > > > > > > > > Can you use sandbox instead, or perhaps sandbox_spl? SPL is really for > > > > > > > > SPL testing. > > > > > > > > > > > > > > Er, I am actually using the sandbox_spl variant. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > > > > > --- > > > > > > > > > Changes since V3: > > > > > > > > > * Add test cases for covering the various capsule generation > > > > > > > > > scenarios. > > > > > > > > > * Add function comments in the mkeficapsule bintool. > > > > > > > > > * Fix the fetch method of the mkeficapsule bintool to enable building > > > > > > > > > the tool. > > > > > > > > > * Add more details about the capsule parameters in the documentation > > > > > > > > > as well as the code. > > > > > > > > > * Fix order of module imports, and addition of blank lines in the > > > > > > > > > capsule.py file. > > > > > > > > > * Use SetContents in the ObtainContents method. > > > > > > > > > > > > > > > > > > configs/sandbox_spl_defconfig | 1 + > > > > > > > > > tools/binman/btool/mkeficapsule.py | 158 ++++++++++++++++++ > > > > > > > > > tools/binman/entries.rst | 37 ++++ > > > > > > > > > tools/binman/etype/capsule.py | 132 +++++++++++++++ > > > > > > > > > tools/binman/ftest.py | 127 ++++++++++++++ > > > > > > > > > tools/binman/test/282_capsule.dts | 18 ++ > > > > > > > > > tools/binman/test/283_capsule_signed.dts | 20 +++ > > > > > > > > > tools/binman/test/284_capsule_conf.dts | 14 ++ > > > > > > > > > tools/binman/test/285_capsule_missing_key.dts | 19 +++ > > > > > > > > > .../binman/test/286_capsule_missing_index.dts | 17 ++ > > > > > > > > > .../binman/test/287_capsule_missing_guid.dts | 17 ++ > > > > > > > > > .../test/288_capsule_missing_payload.dts | 17 ++ > > > > > > > > > tools/binman/test/289_capsule_missing.dts | 17 ++ > > > > > > > > > tools/binman/test/290_capsule_version.dts | 19 +++ > > > > > > > > > tools/binman/test/capsule_cfg.txt | 6 + > > > > > > > > > 15 files changed, 619 insertions(+) > > > > > > > > > create mode 100644 tools/binman/btool/mkeficapsule.py > > > > > > > > > create mode 100644 tools/binman/etype/capsule.py > > > > > > > > > create mode 100644 tools/binman/test/282_capsule.dts > > > > > > > > > create mode 100644 tools/binman/test/283_capsule_signed.dts > > > > > > > > > create mode 100644 tools/binman/test/284_capsule_conf.dts > > > > > > > > > create mode 100644 tools/binman/test/285_capsule_missing_key.dts > > > > > > > > > create mode 100644 tools/binman/test/286_capsule_missing_index.dts > > > > > > > > > create mode 100644 tools/binman/test/287_capsule_missing_guid.dts > > > > > > > > > create mode 100644 tools/binman/test/288_capsule_missing_payload.dts > > > > > > > > > create mode 100644 tools/binman/test/289_capsule_missing.dts > > > > > > > > > create mode 100644 tools/binman/test/290_capsule_version.dts > > > > > > > > > create mode 100644 tools/binman/test/capsule_cfg.txt > > > > > > > > > > > > > > > > This looks pretty good to me. Some nits below > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig > > > > > > > > > index dd848c57c6..2fcc789347 100644 > > > > > > > > > --- a/configs/sandbox_spl_defconfig > > > > > > > > > +++ b/configs/sandbox_spl_defconfig > > > > > > > > > @@ -248,3 +248,4 @@ CONFIG_UNIT_TEST=y > > > > > > > > > CONFIG_SPL_UNIT_TEST=y > > > > > > > > > CONFIG_UT_TIME=y > > > > > > > > > CONFIG_UT_DM=y > > > > > > > > > +CONFIG_TOOLS_MKEFICAPSULE=y > > > > > > > > > > > > > > > > Why enabling this here? I don't think it is needed in sandbox_spl, but > > > > > > > > in any case it should be in a different patch if needed. > > > > > > > > > > > > > > The binman tests run on the sandbox_spl variant. When running the > > > > > > > capsule generation tests, the mkeficapsule tool should be present on > > > > > > > the board variant no? > > > > > > > > > > > > Can we run this on the 'sandbox' variant instead? > > > > > > > > > > The capsule tests can be run on sandbox. But the change in the > > > > > sandbox_spl_defconfig is for adding support for the capsule tests > > > > > which are run as part of the binman test suite in CI. So it would be a > > > > > > > > The binman tests are run separately, in 'Run binman, buildman, dtoc, > > > > Kconfig and patman testsuites' in gitlab. > > > > > > Yes, and that uses the sandbox_spl variant build for running the > > > tests. Which is why I added building the mkeficapsule tool for the > > > variant. > > > > Not really. It uses sandbox_spl to obtain the pylibfdt library, that's all. > > > > The tests should use sandbox. By enabling CONFIG_BINMAN you will get > > pylibfdt anyway. > > I will check on this point. > > > > > > > > > > > > > > > question of whether the rest of the binman tests in CI can be run on > > > > > the sandbox variant. Or are there any specific set of tests which need > > > > > to be run on the sandbox_spl variant? > > > > > > > > Just the ones that need SPL. If you type 'make qcheck' you will see > > > > most/all of the tests, including the sandbox_spl ones. > > > > > > Can we not just build the mkeficapsule tool in the sandbox_spl > > > variant. That keeps things simple. > > > > Why is it simple? It is actually a pain, because the vast majority of > > tests only need sandbox. We use sandbox_spl for tests that do > > something in SPL and need that phase of U-Boot to perform some sort of > > test. So far as I can tell, the EFI capsule thing works entirely in > > U-Boot proper. > > > > > > > > > > > > > > > > > > > > > > > > > > [..] > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > + def ReadNode(self): > > > > > > > > > + super().ReadNode() > > > > > > > > > + > > > > > > > > > + self.cfg_file = fdt_util.GetString(self._node, 'cfg-file') > > > > > > > > > + if not self.cfg_file: > > > > > > > > > + self.image_index = fdt_util.GetInt(self._node, 'image-index') > > > > > > > > > + if not self.image_index: > > > > > > > > > + self.Raise('mkeficapsule must be provided an Image Index') > > > > > > > > > + > > > > > > > > > + self.image_guid = fdt_util.GetString(self._node, 'image-type-id') > > > > > > > > > + if not self.image_guid: > > > > > > > > > + self.Raise('mkeficapsule must be provided an Image GUID') > > > > > > > > > > > > > > > > Use self.required_props = ['image-type-id', ...] in your __init__(). > > > > > > > > Then this is automatic > > > > > > > > > > > > > > I should have clarified this during the earlier version itself. So > > > > > > > these parameters are mandatory only when not using the config file. In > > > > > > > the scenario of generating the capsules through config files, all > > > > > > > these parameters are provided through the config file. Hence these > > > > > > > explicit checks. > > > > > > > > > > > > Hmm, I think we should consider having two different etypes, then. It > > > > > > seems in fact that your entry type is doing 2-3 different things? > > > > > > > > > > I am wondering if having two different etypes might get confusing for the users. > > > > > > > > Maybe, but I'm already confused. > > > > > > > > > > > > > > > > > > > > > [..] > > > > > > > > > > > > > > What if some of the inputs are missing? Does this handle missing blobs? > > > > > > > > > > > > > > Any missing input parameters are checked earlier itself. > > > > > > > > > > > > > > > > + if self.cfg_file: > > > > > > > > > + return self.mkeficapsule.capsule_cfg_file(self.cfg_file) > > > > > > > > > + elif self.auth: > > > > > > > > > + return self.mkeficapsule.cmdline_auth_capsule(self.image_index, > > > > > > > > > + self.image_guid, > > > > > > > > > + self.hardware_instance, > > > > > > > > > + self.monotonic_count, > > > > > > > > > + self.private_key, > > > > > > > > > + self.pub_key_cert, > > > > > > > > > + self.payload, > > > > > > > > > + self.capsule_fname, > > > > > > > > > + self.fw_version) > > > > > > > > > + else: > > > > > > > > > + return self.mkeficapsule.cmdline_capsule(self.image_index, > > > > > > > > > + self.image_guid, > > > > > > > > > + self.hardware_instance, > > > > > > > > > + self.payload, > > > > > > > > > + self.capsule_fname, > > > > > > > > > + self.fw_version) > > > > > > > > > > > > Here is where I wonder whether you are putting too much in a single > > > > > > etype. Here there are three different cases. Should we have 3 etypes? > > > > > > > > > > Basically the result in all the three cases is the same -- generation > > > > > of a capsule file. Just that the input parameters being passed for > > > > > generation are slightly different. There can be multiple entry types > > > > > for these, but like I mentioned above, would having multiple entry > > > > > types not be confusing for the users? > > > > > > > > So can we drop the use of a cfg file? What is the difference between > > > > the second two? If you had added comments I would not have had to ask. > > > > > > Using a config file to specify all the parameters needed for building > > > capsule(s) is a good feature to have. It also results in generation of > > > all the needed capsules through a single invocation of the > > > mkeficapsule command, instead of multiple calls per capsule file. > > > > Does that matter? I don't really have an objection to it, except in > > that it makes the entry type confusing. Do we need to support this > > mode of operation in binman? Perhaps EFI capsule can do what ever > > other enrty type does and specify the params in the Binman node? You > > can still use the config file for other uses, but I don't see any use > > for it in binman. > > > > If you want multiple capsules, wouldn't that be done with multiple > > capsule nodes in the image definition? > > > > > > > > > > > > > > > > > > > > > > > > > > [..] > > > > > > > > > > > > > > > > > > > > > > We really should not have GUIDs in the code...they are a mess. > > > > > > > > > > > > > > You want the UEFI capsule generation to happen through binman, and not > > > > > > > mention GUIDs. That ain't happening :) > > > > > > > > > > > > I just don't want them open-coded. They are meaningless gibberish that > > > > > > no one can understand. Use #define or some other way to give them a > > > > > > name. > > > > > > > > > > Yes, I will move the GUIDS under a variable. > > > > > > > > > > > > > > > > > If I wrote: > > > > > > > > > > > > writel(0x09812374, 0x8723728) > > > > > > > > > > > > you would have the same comment. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + # Image GUID specified in the DTS > > > > > > > > > + self.image_guid = "52cfd7092007104791d108469b7fe9c8" > > > > > > > > > + self.fmp_signature = "4d535331" > > > > > > > > > + self.fmp_size = "10" > > > > > > > > > + self.fmp_fw_version = "02" > > > > > > > > > > > > > > > > These should really be local vars, not members. > > > > > > > > > > > > > > Okay > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > + self.capsule_data = tools.read_file(self.capsule_fname) > > > > > > > > > > > > > > > > Pass the data in here and then you don't need to read the file > > > > > > > > > > > > > > So the file needs to be read here since the actual capsule generation > > > > > > > tool(tools/mkeficapsule) does not return any capsule data. Instead, > > > > > > > the data gets written to the capsule file, and the tool just returns a > > > > > > > pass/fail status. > > > > > > > > > > > > Sure, but you can read that data in the caller to this functoin. > > > > > > > > > > Yes, the data can be read in the caller, but that would mean > > > > > duplicating the read in four functions. Instead the file read is > > > > > happening in one place where the data is being used. > > > > > > > > What do you mean? The data is returned from the call to > > > > self._DoReadFile(), the so pattern is: > > > > > > > > data = self._DoReadFile(...) > > > > > > > > You should be able to see that throughout ftest.py > > > > > > > > What exactly is the confusion here? > > > > > > No confusion. Again, this does not work when generating capsules > > > through a config file. > > > > OK let's drop the config file from binman. We don't need it and I'm > > tired of the back and forth. > > Sorry about that, but like I mentioned in my other reply, we would be > needing support for generating capsules through the config file. Can > we not work around the testing in binman for this feature. Like I > mentioned earlier, this will be getting tested as part of the larger > efi capsule update feature testing. We can worry about it later when all the other issues are resolved and this series is applied. Ideally that would be this week, since we have already hit rc1. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > + self.assertEqual(self.capsule_guid, self.capsule_data.hex()[:32]) > > > > > > > > > + self.assertEqual(self.image_guid, self.capsule_data.hex()[96:128]) > > > > > > > > > + > > > > > > > > > + if version_check: > > > > > > > > > + self.assertEqual(self.fmp_signature, self.capsule_data.hex()[184:192]) > > > > > > > > > + self.assertEqual(self.fmp_size, self.capsule_data.hex()[192:194]) > > > > > > > > > + self.assertEqual(self.fmp_fw_version, self.capsule_data.hex()[200:202]) > > > > > > > > > + > > > > > > > > > + if signed_capsule: > > > > > > > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[4770:4778]) > > > > > > > > > > > > > > > > Where do these integer offsets come from? Please add a comment > > > > > > > > > > > > > > So, these are simply offsets in the output capsule file, which get > > > > > > > impacted based on the contents being put in the capsule, like > > > > > > > presence/absence of optional headers. I don't think putting a comment > > > > > > > really wll add any value, because these offsets are variable. > > > > > > > > > > > > OK then please add a comment to that effect, as well as how to figure > > > > > > them out when things change. > > > > > > > > > > Okay > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + elif version_check: > > > > > > > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[216:224]) > > > > > > > > > + else: > > > > > > > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[184:192]) > > > > > > > > > + > > > > > > > > > + def _GenCapsuleCfgPayload(self, fname, contents): > > > > > > > > > + capsule_dir = '/tmp/capsules/' > > > > > > > > > > > > > > > > You can't write to /tmp > > > > > > > > > > > > > > > > Please use self._indir for input files - see how other tests do it > > > > > > > > > > > > > > For all other tests, I am indeed using _indir and outdir. But for > > > > > > > generation of capsules through a config file, we need to specify the > > > > > > > location where the output capsule file will be written to. Which is > > > > > > > the reason for the /tmp/capsules/. We are using this directory for > > > > > > > collating all capsule related files. > > > > > > > > > > > > Well, sorry, you can't do that. I think you should provide a relative > > > > > > path, rather than absolute...that should solve the problem > > > > > > > > > > Using absolute paths is only being done in the case of testing capsule > > > > > generation through config file -- all the rest of the test cases are > > > > > using relative paths for the input file and the capsule file. For the > > > > > generation of capsules through the config file, the paths either need > > > > > to be absolute, or relative to the directory from which the > > > > > mkeficapsule tool is being invoked. And I believe that the binman > > > > > test is creating temporary directories at runtime for input and output > > > > > files. > > > > > > > > OK so let's drop the cfg file stuff. It doesn't seem to be needed. > > > > > > Instead of dropping support for generating capsules through a cfg file > > > in binman, I would say drop test coverage in binman for that case. We > > > can test this feature, that is not an issue, but you insist on using > > > the binman input and output directories which get created at runtime. > > > We cannot test the capsule generation with a cfg file with that > > > restriction. > > > > No. > > > > > > > > > > > > > > > > > > > > > > What is the issue you see in using the /tmp/capsules/ path for > > > > > generation of capsules. I see directories being created with relevant > > > > > files under /opt for other tests. If you would want the capsules > > > > > directory to be in some other location, I can change that. > > > > > > > > Binman tests should use the input and output directories, so please > > > > figure out how to do this. If you drop the cfg file then it should be > > > > easy. > > > > > > If I could do it, I would not spend this time discussing with you. > > > Like I said above, if you insist on not using a known location for the > > > input and output files, maybe we can drop the test for cfg file -- > > > this functionality does get tested as part of the capsule update > > > feature testing. I would argue that generating capsules using a cfg > > > file is beneficial and be retained. > > > > As above. > > > > > > > > > > > > > > > > > > > > > > > > > > [..] > > > > > > > > > > > > > > > > > > > > > > Please can you use your own test data, like EFI_DATA ? Also if you > > > > > > > > declare it as a binary string you can drop the call. > > > > > > > > > > > > > > > > For example: > > > > > > > > > > > > > > > > EFI_DATA = b'efi' > > > > > > > > > > > > > > I don't know why text_data cannot be used, but I will add the EFI_DATA. > > > > > > > > > > > > Well firstly it is not a 'bytes' string. Secondly you may as well have > > > > > > your own as we have done with other etypes. > > > > > > > > > > Okay > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > + TestFunctional._MakeInputFile('payload.txt', self.payload_data) > > > > > > > > > + > > > > > > > > > + self._DoReadFile('282_capsule.dts') > > > > > > > > > > > > > > > > data = self... > > > > > > > > > > > > > > Please see above. We need to read the capsule file. This applies for > > > > > > > all the related comments about using the data = self._DoReadFile... > > > > > > > > > > > > That needs to be fixed, since the output file should be the capsule. > > > > > > Why would the output file be anything else?? > > > > > > > > > > The output file is indeed a capsule, but that is being generated by > > > > > the capsule generation tool which gets called from the bintool. So the > > > > > mkeficapsule tool that the bintool calls results in generation of the > > > > > capsule file. In that way, this does not map directly to the concept > > > > > of an image in binman. And I was wondering if I should introduce a new > > > > > property like 'dummy-image', so that the resultant <image>.bin and > > > > > <image>.map files are not created -- they really are not needed in the > > > > > case of capsule generation. Would you be fine with an introduction of > > > > > such a property? > > > > > > > > Eek, no. > > > > > > > > I thought the output file from binman (image.bin in most tests) was an > > > > EFI capsule. Is that not the case? If not, what exactly is it? > > > > > > In this case, it is not so. Which is why I proposed the above > > > property. What happens is that the bintool calls the actual capsule > > > generation tool, and that tool writes the capsule contents to a file > > > which is provided to the tool as a parameter. Binman does not have to > > > create the image.bin file in this case, since the mkeficapsule tool > > > has already created the capsule file -- creating the image.bin and > > > image.map is superfluous here. > > > > I think you are missing something here. The tools that binman uses are > > there to generate things that the binman description wants. The EFI > > capsule is just another one of those. > > > > We should rename 'capsule' to 'efi-capsule', I think, since there > > might be other types of capsule. > > > > In theory I could say: > > > > binman { > > u-boot-spl { > > }; > > efi-capsule { > > // things we want in the capsule > > u-boot { > > } > > } > > > > and it should produce an image with SPL at the start followed by the > > capsule. The capsule is allowed to generate data, not create the whole > > image. Perhaps that is the blocker here? > > > > I see now that your capsule entry type is wonky. It needs to accept > > properties (not filenames!) and use those to package up the contents, > > which should be subnodes of itself. > > Okay. I will take a look at how mkimage is called. I had checked the > intel_ifwi entry type for reference, and I did see use of the filename > property there. Yes sections can have filenames iwc the content is written to the filename. But it is still handled internally by binman. There might be a misunderstanding of how binman works...did you look through all the docs? > > > > > Probably you should look at how mkimage does it...but you essentially > > call self.collect_contents_to_file() to pick up the data. Your capsule > > should probably subclass entry_section, rather than Entry, since > > entry_Section handles some other things for you, like missing blobs. > > Okay, I will check. > > -sughosh > > > > > Looking at your last patch I think this is the confusion, and why this > > has been so hard for me to wrap my head around. > Regards, Simon
hi Simon, On Wed, 26 Jul 2023 at 04:06, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Thu, 20 Jul 2023 at 03:20, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > hi Simon, > > > > On Thu, 20 Jul 2023 at 00:41, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Wed, 19 Jul 2023 at 02:42, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > hi Simon, > > > > > > > > On Wed, 19 Jul 2023 at 06:41, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > Hi Sughosh, > > > > > > > > > > On Mon, 17 Jul 2023 at 04:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > > > hi Simon, > > > > > > > > > > > > On Sun, 16 Jul 2023 at 05:12, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > > > On Sat, 15 Jul 2023 at 07:46, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > > > > > > > Add support in binman for generating capsules. The capsule parameters > > > > > > > > can be specified either through a config file or through the capsule > > > > > > > > binman entry. Also add test cases in binman for capsule generation, > > > > > > > > and enable this testing on the sandbox_spl variant. > > > > > > > > > > > > > > Can you use sandbox instead, or perhaps sandbox_spl? SPL is really for > > > > > > > SPL testing. > > > > > > > > > > > > Er, I am actually using the sandbox_spl variant. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > > > > --- > > > > > > > > Changes since V3: > > > > > > > > * Add test cases for covering the various capsule generation > > > > > > > > scenarios. > > > > > > > > * Add function comments in the mkeficapsule bintool. > > > > > > > > * Fix the fetch method of the mkeficapsule bintool to enable building > > > > > > > > the tool. > > > > > > > > * Add more details about the capsule parameters in the documentation > > > > > > > > as well as the code. > > > > > > > > * Fix order of module imports, and addition of blank lines in the > > > > > > > > capsule.py file. > > > > > > > > * Use SetContents in the ObtainContents method. > > > > > > > > > > > > > > > > configs/sandbox_spl_defconfig | 1 + > > > > > > > > tools/binman/btool/mkeficapsule.py | 158 ++++++++++++++++++ > > > > > > > > tools/binman/entries.rst | 37 ++++ > > > > > > > > tools/binman/etype/capsule.py | 132 +++++++++++++++ > > > > > > > > tools/binman/ftest.py | 127 ++++++++++++++ > > > > > > > > tools/binman/test/282_capsule.dts | 18 ++ > > > > > > > > tools/binman/test/283_capsule_signed.dts | 20 +++ > > > > > > > > tools/binman/test/284_capsule_conf.dts | 14 ++ > > > > > > > > tools/binman/test/285_capsule_missing_key.dts | 19 +++ > > > > > > > > .../binman/test/286_capsule_missing_index.dts | 17 ++ > > > > > > > > .../binman/test/287_capsule_missing_guid.dts | 17 ++ > > > > > > > > .../test/288_capsule_missing_payload.dts | 17 ++ > > > > > > > > tools/binman/test/289_capsule_missing.dts | 17 ++ > > > > > > > > tools/binman/test/290_capsule_version.dts | 19 +++ > > > > > > > > tools/binman/test/capsule_cfg.txt | 6 + > > > > > > > > 15 files changed, 619 insertions(+) > > > > > > > > create mode 100644 tools/binman/btool/mkeficapsule.py > > > > > > > > create mode 100644 tools/binman/etype/capsule.py > > > > > > > > create mode 100644 tools/binman/test/282_capsule.dts > > > > > > > > create mode 100644 tools/binman/test/283_capsule_signed.dts > > > > > > > > create mode 100644 tools/binman/test/284_capsule_conf.dts > > > > > > > > create mode 100644 tools/binman/test/285_capsule_missing_key.dts > > > > > > > > create mode 100644 tools/binman/test/286_capsule_missing_index.dts > > > > > > > > create mode 100644 tools/binman/test/287_capsule_missing_guid.dts > > > > > > > > create mode 100644 tools/binman/test/288_capsule_missing_payload.dts > > > > > > > > create mode 100644 tools/binman/test/289_capsule_missing.dts > > > > > > > > create mode 100644 tools/binman/test/290_capsule_version.dts > > > > > > > > create mode 100644 tools/binman/test/capsule_cfg.txt > > > > > > > > > > > > > > This looks pretty good to me. Some nits below > > > > > > > > > > > > > > > > > > > > > > > diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig > > > > > > > > index dd848c57c6..2fcc789347 100644 > > > > > > > > --- a/configs/sandbox_spl_defconfig > > > > > > > > +++ b/configs/sandbox_spl_defconfig > > > > > > > > @@ -248,3 +248,4 @@ CONFIG_UNIT_TEST=y > > > > > > > > CONFIG_SPL_UNIT_TEST=y > > > > > > > > CONFIG_UT_TIME=y > > > > > > > > CONFIG_UT_DM=y > > > > > > > > +CONFIG_TOOLS_MKEFICAPSULE=y > > > > > > > > > > > > > > Why enabling this here? I don't think it is needed in sandbox_spl, but > > > > > > > in any case it should be in a different patch if needed. > > > > > > > > > > > > The binman tests run on the sandbox_spl variant. When running the > > > > > > capsule generation tests, the mkeficapsule tool should be present on > > > > > > the board variant no? > > > > > > > > > > Can we run this on the 'sandbox' variant instead? > > > > > > > > The capsule tests can be run on sandbox. But the change in the > > > > sandbox_spl_defconfig is for adding support for the capsule tests > > > > which are run as part of the binman test suite in CI. So it would be a > > > > > > The binman tests are run separately, in 'Run binman, buildman, dtoc, > > > Kconfig and patman testsuites' in gitlab. > > > > Yes, and that uses the sandbox_spl variant build for running the > > tests. Which is why I added building the mkeficapsule tool for the > > variant. > > Not really. It uses sandbox_spl to obtain the pylibfdt library, that's all. > > The tests should use sandbox. By enabling CONFIG_BINMAN you will get > pylibfdt anyway. I am not sure if I am missing something. But if I do not build the mkeficapsule tool as part of the sandbox_spl variant, I get errors in CI on the binman tests. And I believe that is because binman is being run with the '--toolpath /tmp/sandbox_spl/tools', which is built for the sandbox_spl variant. So if the mkeficapsule tool is not built for the sandbox_spl variant, this results in the binman capsule tests to fail in CI. I see this at least with the azure pipeline. -sughosh > > > > > > > > > > question of whether the rest of the binman tests in CI can be run on > > > > the sandbox variant. Or are there any specific set of tests which need > > > > to be run on the sandbox_spl variant? > > > > > > Just the ones that need SPL. If you type 'make qcheck' you will see > > > most/all of the tests, including the sandbox_spl ones. > > > > Can we not just build the mkeficapsule tool in the sandbox_spl > > variant. That keeps things simple. > > Why is it simple? It is actually a pain, because the vast majority of > tests only need sandbox. We use sandbox_spl for tests that do > something in SPL and need that phase of U-Boot to perform some sort of > test. So far as I can tell, the EFI capsule thing works entirely in > U-Boot proper. > > > > > > > > > > > > > > > > > > > > [..] > > > > > > > > > > > > > > > > > > > > + > > > > > > > > + def ReadNode(self): > > > > > > > > + super().ReadNode() > > > > > > > > + > > > > > > > > + self.cfg_file = fdt_util.GetString(self._node, 'cfg-file') > > > > > > > > + if not self.cfg_file: > > > > > > > > + self.image_index = fdt_util.GetInt(self._node, 'image-index') > > > > > > > > + if not self.image_index: > > > > > > > > + self.Raise('mkeficapsule must be provided an Image Index') > > > > > > > > + > > > > > > > > + self.image_guid = fdt_util.GetString(self._node, 'image-type-id') > > > > > > > > + if not self.image_guid: > > > > > > > > + self.Raise('mkeficapsule must be provided an Image GUID') > > > > > > > > > > > > > > Use self.required_props = ['image-type-id', ...] in your __init__(). > > > > > > > Then this is automatic > > > > > > > > > > > > I should have clarified this during the earlier version itself. So > > > > > > these parameters are mandatory only when not using the config file. In > > > > > > the scenario of generating the capsules through config files, all > > > > > > these parameters are provided through the config file. Hence these > > > > > > explicit checks. > > > > > > > > > > Hmm, I think we should consider having two different etypes, then. It > > > > > seems in fact that your entry type is doing 2-3 different things? > > > > > > > > I am wondering if having two different etypes might get confusing for the users. > > > > > > Maybe, but I'm already confused. > > > > > > > > > > > > > > > > > [..] > > > > > > > > > > > > What if some of the inputs are missing? Does this handle missing blobs? > > > > > > > > > > > > Any missing input parameters are checked earlier itself. > > > > > > > > > > > > > > + if self.cfg_file: > > > > > > > > + return self.mkeficapsule.capsule_cfg_file(self.cfg_file) > > > > > > > > + elif self.auth: > > > > > > > > + return self.mkeficapsule.cmdline_auth_capsule(self.image_index, > > > > > > > > + self.image_guid, > > > > > > > > + self.hardware_instance, > > > > > > > > + self.monotonic_count, > > > > > > > > + self.private_key, > > > > > > > > + self.pub_key_cert, > > > > > > > > + self.payload, > > > > > > > > + self.capsule_fname, > > > > > > > > + self.fw_version) > > > > > > > > + else: > > > > > > > > + return self.mkeficapsule.cmdline_capsule(self.image_index, > > > > > > > > + self.image_guid, > > > > > > > > + self.hardware_instance, > > > > > > > > + self.payload, > > > > > > > > + self.capsule_fname, > > > > > > > > + self.fw_version) > > > > > > > > > > Here is where I wonder whether you are putting too much in a single > > > > > etype. Here there are three different cases. Should we have 3 etypes? > > > > > > > > Basically the result in all the three cases is the same -- generation > > > > of a capsule file. Just that the input parameters being passed for > > > > generation are slightly different. There can be multiple entry types > > > > for these, but like I mentioned above, would having multiple entry > > > > types not be confusing for the users? > > > > > > So can we drop the use of a cfg file? What is the difference between > > > the second two? If you had added comments I would not have had to ask. > > > > Using a config file to specify all the parameters needed for building > > capsule(s) is a good feature to have. It also results in generation of > > all the needed capsules through a single invocation of the > > mkeficapsule command, instead of multiple calls per capsule file. > > Does that matter? I don't really have an objection to it, except in > that it makes the entry type confusing. Do we need to support this > mode of operation in binman? Perhaps EFI capsule can do what ever > other enrty type does and specify the params in the Binman node? You > can still use the config file for other uses, but I don't see any use > for it in binman. > > If you want multiple capsules, wouldn't that be done with multiple > capsule nodes in the image definition? > > > > > > > > > > > > > > > > > > > > [..] > > > > > > > > > > > > > > > > > > > We really should not have GUIDs in the code...they are a mess. > > > > > > > > > > > > You want the UEFI capsule generation to happen through binman, and not > > > > > > mention GUIDs. That ain't happening :) > > > > > > > > > > I just don't want them open-coded. They are meaningless gibberish that > > > > > no one can understand. Use #define or some other way to give them a > > > > > name. > > > > > > > > Yes, I will move the GUIDS under a variable. > > > > > > > > > > > > > > If I wrote: > > > > > > > > > > writel(0x09812374, 0x8723728) > > > > > > > > > > you would have the same comment. > > > > > > > > > > > > > > > > > > > > > > > > > > + # Image GUID specified in the DTS > > > > > > > > + self.image_guid = "52cfd7092007104791d108469b7fe9c8" > > > > > > > > + self.fmp_signature = "4d535331" > > > > > > > > + self.fmp_size = "10" > > > > > > > > + self.fmp_fw_version = "02" > > > > > > > > > > > > > > These should really be local vars, not members. > > > > > > > > > > > > Okay > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > + self.capsule_data = tools.read_file(self.capsule_fname) > > > > > > > > > > > > > > Pass the data in here and then you don't need to read the file > > > > > > > > > > > > So the file needs to be read here since the actual capsule generation > > > > > > tool(tools/mkeficapsule) does not return any capsule data. Instead, > > > > > > the data gets written to the capsule file, and the tool just returns a > > > > > > pass/fail status. > > > > > > > > > > Sure, but you can read that data in the caller to this functoin. > > > > > > > > Yes, the data can be read in the caller, but that would mean > > > > duplicating the read in four functions. Instead the file read is > > > > happening in one place where the data is being used. > > > > > > What do you mean? The data is returned from the call to > > > self._DoReadFile(), the so pattern is: > > > > > > data = self._DoReadFile(...) > > > > > > You should be able to see that throughout ftest.py > > > > > > What exactly is the confusion here? > > > > No confusion. Again, this does not work when generating capsules > > through a config file. > > OK let's drop the config file from binman. We don't need it and I'm > tired of the back and forth. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > + self.assertEqual(self.capsule_guid, self.capsule_data.hex()[:32]) > > > > > > > > + self.assertEqual(self.image_guid, self.capsule_data.hex()[96:128]) > > > > > > > > + > > > > > > > > + if version_check: > > > > > > > > + self.assertEqual(self.fmp_signature, self.capsule_data.hex()[184:192]) > > > > > > > > + self.assertEqual(self.fmp_size, self.capsule_data.hex()[192:194]) > > > > > > > > + self.assertEqual(self.fmp_fw_version, self.capsule_data.hex()[200:202]) > > > > > > > > + > > > > > > > > + if signed_capsule: > > > > > > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[4770:4778]) > > > > > > > > > > > > > > Where do these integer offsets come from? Please add a comment > > > > > > > > > > > > So, these are simply offsets in the output capsule file, which get > > > > > > impacted based on the contents being put in the capsule, like > > > > > > presence/absence of optional headers. I don't think putting a comment > > > > > > really wll add any value, because these offsets are variable. > > > > > > > > > > OK then please add a comment to that effect, as well as how to figure > > > > > them out when things change. > > > > > > > > Okay > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + elif version_check: > > > > > > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[216:224]) > > > > > > > > + else: > > > > > > > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[184:192]) > > > > > > > > + > > > > > > > > + def _GenCapsuleCfgPayload(self, fname, contents): > > > > > > > > + capsule_dir = '/tmp/capsules/' > > > > > > > > > > > > > > You can't write to /tmp > > > > > > > > > > > > > > Please use self._indir for input files - see how other tests do it > > > > > > > > > > > > For all other tests, I am indeed using _indir and outdir. But for > > > > > > generation of capsules through a config file, we need to specify the > > > > > > location where the output capsule file will be written to. Which is > > > > > > the reason for the /tmp/capsules/. We are using this directory for > > > > > > collating all capsule related files. > > > > > > > > > > Well, sorry, you can't do that. I think you should provide a relative > > > > > path, rather than absolute...that should solve the problem > > > > > > > > Using absolute paths is only being done in the case of testing capsule > > > > generation through config file -- all the rest of the test cases are > > > > using relative paths for the input file and the capsule file. For the > > > > generation of capsules through the config file, the paths either need > > > > to be absolute, or relative to the directory from which the > > > > mkeficapsule tool is being invoked. And I believe that the binman > > > > test is creating temporary directories at runtime for input and output > > > > files. > > > > > > OK so let's drop the cfg file stuff. It doesn't seem to be needed. > > > > Instead of dropping support for generating capsules through a cfg file > > in binman, I would say drop test coverage in binman for that case. We > > can test this feature, that is not an issue, but you insist on using > > the binman input and output directories which get created at runtime. > > We cannot test the capsule generation with a cfg file with that > > restriction. > > No. > > > > > > > > > > > > > > > > What is the issue you see in using the /tmp/capsules/ path for > > > > generation of capsules. I see directories being created with relevant > > > > files under /opt for other tests. If you would want the capsules > > > > directory to be in some other location, I can change that. > > > > > > Binman tests should use the input and output directories, so please > > > figure out how to do this. If you drop the cfg file then it should be > > > easy. > > > > If I could do it, I would not spend this time discussing with you. > > Like I said above, if you insist on not using a known location for the > > input and output files, maybe we can drop the test for cfg file -- > > this functionality does get tested as part of the capsule update > > feature testing. I would argue that generating capsules using a cfg > > file is beneficial and be retained. > > As above. > > > > > > > > > > > > > > > > > > > > [..] > > > > > > > > > > > > > > > > > > > Please can you use your own test data, like EFI_DATA ? Also if you > > > > > > > declare it as a binary string you can drop the call. > > > > > > > > > > > > > > For example: > > > > > > > > > > > > > > EFI_DATA = b'efi' > > > > > > > > > > > > I don't know why text_data cannot be used, but I will add the EFI_DATA. > > > > > > > > > > Well firstly it is not a 'bytes' string. Secondly you may as well have > > > > > your own as we have done with other etypes. > > > > > > > > Okay > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > + TestFunctional._MakeInputFile('payload.txt', self.payload_data) > > > > > > > > + > > > > > > > > + self._DoReadFile('282_capsule.dts') > > > > > > > > > > > > > > data = self... > > > > > > > > > > > > Please see above. We need to read the capsule file. This applies for > > > > > > all the related comments about using the data = self._DoReadFile... > > > > > > > > > > That needs to be fixed, since the output file should be the capsule. > > > > > Why would the output file be anything else?? > > > > > > > > The output file is indeed a capsule, but that is being generated by > > > > the capsule generation tool which gets called from the bintool. So the > > > > mkeficapsule tool that the bintool calls results in generation of the > > > > capsule file. In that way, this does not map directly to the concept > > > > of an image in binman. And I was wondering if I should introduce a new > > > > property like 'dummy-image', so that the resultant <image>.bin and > > > > <image>.map files are not created -- they really are not needed in the > > > > case of capsule generation. Would you be fine with an introduction of > > > > such a property? > > > > > > Eek, no. > > > > > > I thought the output file from binman (image.bin in most tests) was an > > > EFI capsule. Is that not the case? If not, what exactly is it? > > > > In this case, it is not so. Which is why I proposed the above > > property. What happens is that the bintool calls the actual capsule > > generation tool, and that tool writes the capsule contents to a file > > which is provided to the tool as a parameter. Binman does not have to > > create the image.bin file in this case, since the mkeficapsule tool > > has already created the capsule file -- creating the image.bin and > > image.map is superfluous here. > > I think you are missing something here. The tools that binman uses are > there to generate things that the binman description wants. The EFI > capsule is just another one of those. > > We should rename 'capsule' to 'efi-capsule', I think, since there > might be other types of capsule. > > In theory I could say: > > binman { > u-boot-spl { > }; > efi-capsule { > // things we want in the capsule > u-boot { > } > } > > and it should produce an image with SPL at the start followed by the > capsule. The capsule is allowed to generate data, not create the whole > image. Perhaps that is the blocker here? > > I see now that your capsule entry type is wonky. It needs to accept > properties (not filenames!) and use those to package up the contents, > which should be subnodes of itself. > > Probably you should look at how mkimage does it...but you essentially > call self.collect_contents_to_file() to pick up the data. Your capsule > should probably subclass entry_section, rather than Entry, since > entry_Section handles some other things for you, like missing blobs. > > Looking at your last patch I think this is the confusion, and why this > has been so hard for me to wrap my head around. > > Regards, > Simon
Hi Sughosh, On Tue, 1 Aug 2023 at 06:29, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > hi Simon, > > On Wed, 26 Jul 2023 at 04:06, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Thu, 20 Jul 2023 at 03:20, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > hi Simon, > > > > > > On Thu, 20 Jul 2023 at 00:41, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Sughosh, > > > > > > > > On Wed, 19 Jul 2023 at 02:42, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > hi Simon, > > > > > > > > > > On Wed, 19 Jul 2023 at 06:41, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > On Mon, 17 Jul 2023 at 04:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > > > > > hi Simon, > > > > > > > > > > > > > > On Sun, 16 Jul 2023 at 05:12, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > > > > > On Sat, 15 Jul 2023 at 07:46, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > > > > > > > > > Add support in binman for generating capsules. The capsule parameters > > > > > > > > > can be specified either through a config file or through the capsule > > > > > > > > > binman entry. Also add test cases in binman for capsule generation, > > > > > > > > > and enable this testing on the sandbox_spl variant. > > > > > > > > > > > > > > > > Can you use sandbox instead, or perhaps sandbox_spl? SPL is really for > > > > > > > > SPL testing. > > > > > > > > > > > > > > Er, I am actually using the sandbox_spl variant. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > > > > > --- > > > > > > > > > Changes since V3: > > > > > > > > > * Add test cases for covering the various capsule generation > > > > > > > > > scenarios. > > > > > > > > > * Add function comments in the mkeficapsule bintool. > > > > > > > > > * Fix the fetch method of the mkeficapsule bintool to enable building > > > > > > > > > the tool. > > > > > > > > > * Add more details about the capsule parameters in the documentation > > > > > > > > > as well as the code. > > > > > > > > > * Fix order of module imports, and addition of blank lines in the > > > > > > > > > capsule.py file. > > > > > > > > > * Use SetContents in the ObtainContents method. > > > > > > > > > > > > > > > > > > configs/sandbox_spl_defconfig | 1 + > > > > > > > > > tools/binman/btool/mkeficapsule.py | 158 ++++++++++++++++++ > > > > > > > > > tools/binman/entries.rst | 37 ++++ > > > > > > > > > tools/binman/etype/capsule.py | 132 +++++++++++++++ > > > > > > > > > tools/binman/ftest.py | 127 ++++++++++++++ > > > > > > > > > tools/binman/test/282_capsule.dts | 18 ++ > > > > > > > > > tools/binman/test/283_capsule_signed.dts | 20 +++ > > > > > > > > > tools/binman/test/284_capsule_conf.dts | 14 ++ > > > > > > > > > tools/binman/test/285_capsule_missing_key.dts | 19 +++ > > > > > > > > > .../binman/test/286_capsule_missing_index.dts | 17 ++ > > > > > > > > > .../binman/test/287_capsule_missing_guid.dts | 17 ++ > > > > > > > > > .../test/288_capsule_missing_payload.dts | 17 ++ > > > > > > > > > tools/binman/test/289_capsule_missing.dts | 17 ++ > > > > > > > > > tools/binman/test/290_capsule_version.dts | 19 +++ > > > > > > > > > tools/binman/test/capsule_cfg.txt | 6 + > > > > > > > > > 15 files changed, 619 insertions(+) > > > > > > > > > create mode 100644 tools/binman/btool/mkeficapsule.py > > > > > > > > > create mode 100644 tools/binman/etype/capsule.py > > > > > > > > > create mode 100644 tools/binman/test/282_capsule.dts > > > > > > > > > create mode 100644 tools/binman/test/283_capsule_signed.dts > > > > > > > > > create mode 100644 tools/binman/test/284_capsule_conf.dts > > > > > > > > > create mode 100644 tools/binman/test/285_capsule_missing_key.dts > > > > > > > > > create mode 100644 tools/binman/test/286_capsule_missing_index.dts > > > > > > > > > create mode 100644 tools/binman/test/287_capsule_missing_guid.dts > > > > > > > > > create mode 100644 tools/binman/test/288_capsule_missing_payload.dts > > > > > > > > > create mode 100644 tools/binman/test/289_capsule_missing.dts > > > > > > > > > create mode 100644 tools/binman/test/290_capsule_version.dts > > > > > > > > > create mode 100644 tools/binman/test/capsule_cfg.txt > > > > > > > > > > > > > > > > This looks pretty good to me. Some nits below > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig > > > > > > > > > index dd848c57c6..2fcc789347 100644 > > > > > > > > > --- a/configs/sandbox_spl_defconfig > > > > > > > > > +++ b/configs/sandbox_spl_defconfig > > > > > > > > > @@ -248,3 +248,4 @@ CONFIG_UNIT_TEST=y > > > > > > > > > CONFIG_SPL_UNIT_TEST=y > > > > > > > > > CONFIG_UT_TIME=y > > > > > > > > > CONFIG_UT_DM=y > > > > > > > > > +CONFIG_TOOLS_MKEFICAPSULE=y > > > > > > > > > > > > > > > > Why enabling this here? I don't think it is needed in sandbox_spl, but > > > > > > > > in any case it should be in a different patch if needed. > > > > > > > > > > > > > > The binman tests run on the sandbox_spl variant. When running the > > > > > > > capsule generation tests, the mkeficapsule tool should be present on > > > > > > > the board variant no? > > > > > > > > > > > > Can we run this on the 'sandbox' variant instead? > > > > > > > > > > The capsule tests can be run on sandbox. But the change in the > > > > > sandbox_spl_defconfig is for adding support for the capsule tests > > > > > which are run as part of the binman test suite in CI. So it would be a > > > > > > > > The binman tests are run separately, in 'Run binman, buildman, dtoc, > > > > Kconfig and patman testsuites' in gitlab. > > > > > > Yes, and that uses the sandbox_spl variant build for running the > > > tests. Which is why I added building the mkeficapsule tool for the > > > variant. > > > > Not really. It uses sandbox_spl to obtain the pylibfdt library, that's all. > > > > The tests should use sandbox. By enabling CONFIG_BINMAN you will get > > pylibfdt anyway. > > I am not sure if I am missing something. But if I do not build the > mkeficapsule tool as part of the sandbox_spl variant, I get errors in > CI on the binman tests. And I believe that is because binman is being > run with the '--toolpath /tmp/sandbox_spl/tools', which is built for > the sandbox_spl variant. So if the mkeficapsule tool is not built for > the sandbox_spl variant, this results in the binman capsule tests to > fail in CI. I see this at least with the azure pipeline. Yes, alll sandbox variants should build all tools, I believe. The mkeficapsule tool should really be built by all boards, right? It also needs to appear in the u-boot-tools debian package too, for example. [..] Regards, Simon
diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig index dd848c57c6..2fcc789347 100644 --- a/configs/sandbox_spl_defconfig +++ b/configs/sandbox_spl_defconfig @@ -248,3 +248,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..ba6b666714 --- /dev/null +++ b/tools/binman/btool/mkeficapsule.py @@ -0,0 +1,158 @@ +# 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 + -f, --cfg-file <config file> config file with capsule parameters + -h, --help print a help message + +""" + +from binman import bintool + +class Bintoolmkeficapsule(bintool.Bintool): + """Handles the 'mkeficapsule' tool + + This bintool is used for generating the EFI capsules. The + capsule generation parameters can either be specified through + command-line, or through a config file. + + """ + def __init__(self, name): + super().__init__(name, 'mkeficapsule tool for generating capsules') + + def capsule_cfg_file(self, cfg_file): + """Generate a capsule reading parameters from config file + + Args: + cfg_file (str): Path to the config file + + Returns: + str: Tool output + """ + + args = [ + f'--cfg-file={cfg_file}' + ] + return self.run_cmd(*args) + + def cmdline_capsule(self, image_index, image_guid, hardware_instance, + payload, output_fname, version=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 + version (int): Image version (Optional) + + Returns: + str: Tool output + """ + + if version: + args = [ + f'--index={image_index}', + f'--fw-version={version}', + f'--guid={image_guid}', + f'--instance={hardware_instance}', + payload, + output_fname + ] + else: + args = [ + f'--index={image_index}', + f'--guid={image_guid}', + f'--instance={hardware_instance}', + payload, + output_fname + ] + + return self.run_cmd(*args) + + def cmdline_auth_capsule(self, image_index, image_guid, hardware_instance, + monotonic_count, priv_key, pub_key, + payload, output_fname, version=0): + """Generate a signed 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 + monotonic_count (int): Count used when signing an image + priv_key (str): Path to the private key + pub_key(str): Path to the public key + payload (str): Path to the input payload image + output_fname (str): Path to the output capsule file + version (int): Image version (Optional) + + Returns: + str: Tool output + """ + + if version: + args = [ + f'--index={image_index}', + f'--guid={image_guid}', + f'--instance={hardware_instance}', + f'--monotonic-count={monotonic_count}', + f'--private-key={priv_key}', + f'--certificate={pub_key}', + f'--fw-version={version}', + payload, + output_fname + ] + else: + args = [ + f'--index={image_index}', + f'--guid={image_guid}', + f'--instance={hardware_instance}', + f'--monotonic-count={monotonic_count}', + f'--private-key={priv_key}', + f'--certificate={pub_key}', + payload, + output_fname + ] + + 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 b71af801fd..523c8222f5 100644 --- a/tools/binman/entries.rst +++ b/tools/binman/entries.rst @@ -283,6 +283,43 @@ entry; similarly for SPL. +.. _etype_capsule: + +Entry: capsule: Entry for generating EFI Capsule files +------------------------------------------------------ + +This is an entry for generating EFI capsules. + +The parameters needed for generation of the capsules can either be +provided separately, or through a config file. + +Properties / Entry arguments: + - cfg-file: Config file for providing capsule + parameters. These are parameters needed for generating the + capsules. The parameters can be listed by running the + './tools/mkeficapsule -h' command. + - 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. + - monotomic-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. + - filename: Path to the input(payload) file. File can be any + format, a binary or an elf, platform specific. + - capsule: Path to the output capsule file. A capsule is a + continous set of data as defined by the EFI specification. Refer + to the specification for more details. + + + .. _etype_cbfs: Entry: cbfs: Coreboot Filesystem (CBFS) diff --git a/tools/binman/etype/capsule.py b/tools/binman/etype/capsule.py new file mode 100644 index 0000000000..46e5507704 --- /dev/null +++ b/tools/binman/etype/capsule.py @@ -0,0 +1,132 @@ +# SPDX-License-Identifier: GPL-2.0+ +# Copyright (c) 2023 Linaro Limited +# +# Entry-type module for producing a capsule +# + +import os + +from binman.entry import Entry +from dtoc import fdt_util +from u_boot_pylib import tools + +class Entry_capsule(Entry): + """Entry for generating EFI capsules + + This is an entry for generating EFI capsules. + + The parameters needed for generation of the capsules can + either be provided separately, or through a config file. + + Properties / Entry arguments: + - cfg-file: Config file for providing capsule + parameters. These are parameters needed for generating the + capsules. The parameters can be listed by running the + './tools/mkeficapsule -h' command. + - 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. + - monotomic-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. + - filename: Path to the input(payload) file. File can be any + format, a binary or an elf, platform specific. + - capsule: Path to the output capsule file. A capsule is a + continous set of data as defined by the EFI specification. Refer + to the specification for more details. + + """ + def __init__(self, section, etype, node): + super().__init__(section, etype, node) + self.image_index = 0 + self.image_guid = "" + self.hardware_instance = 0 + self.monotonic_count = 0 + self.fw_version = 0 + self.private_key = "" + self.pub_key_cert = "" + self.auth = 0 + self.payload = "" + self.capsule_fname = "" + + def ReadNode(self): + super().ReadNode() + + self.cfg_file = fdt_util.GetString(self._node, 'cfg-file') + if not self.cfg_file: + self.image_index = fdt_util.GetInt(self._node, 'image-index') + if not self.image_index: + self.Raise('mkeficapsule must be provided an Image Index') + + self.image_guid = fdt_util.GetString(self._node, 'image-type-id') + if not self.image_guid: + self.Raise('mkeficapsule must be provided an Image GUID') + + 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.private_key = fdt_util.GetString(self._node, 'private-key') + self.pub_key_cert = fdt_util.GetString(self._node, 'pub-key-cert') + + if ((self.private_key and not self.pub_key_cert) or (self.pub_key_cert and not self.private_key)): + self.Raise('Both private-key and public key Certificate need to be provided') + elif not (self.private_key and self.pub_key_cert): + self.auth = 0 + else: + self.auth = 1 + + self.payload = fdt_util.GetString(self._node, 'filename') + if not self.payload: + self.Raise('mkeficapsule must be provided an input filename(payload)') + + if not os.path.isabs(self.payload): + self.payload_path = tools.get_input_filename(self.payload) + if not os.path.exists(self.payload_path): + self.Raise('Cannot resolve path to the input filename(payload)') + else: + self.payload = self.payload_path + + self.capsule_fname = fdt_util.GetString(self._node, 'capsule') + if not self.capsule_fname: + self.Raise('Specify the output capsule file') + + if not os.path.isabs(self.capsule_fname): + self.capsule_path = tools.get_output_filename(self.capsule_fname) + self.capsule_fname = self.capsule_path + + def _GenCapsule(self): + if self.cfg_file: + return self.mkeficapsule.capsule_cfg_file(self.cfg_file) + elif self.auth: + return self.mkeficapsule.cmdline_auth_capsule(self.image_index, + self.image_guid, + self.hardware_instance, + self.monotonic_count, + self.private_key, + self.pub_key_cert, + self.payload, + self.capsule_fname, + self.fw_version) + else: + return self.mkeficapsule.cmdline_capsule(self.image_index, + self.image_guid, + self.hardware_instance, + self.payload, + self.capsule_fname, + self.fw_version) + + def ObtainContents(self): + self.SetContents(tools.to_bytes(self._GenCapsule())) + return True + + def AddBintools(self, btools): + self.mkeficapsule = self.AddBintool(btools, 'mkeficapsule') diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 43b4f850a6..f5dd62d028 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -6676,6 +6676,133 @@ fdt fdtmap Extract the devicetree blob from the fdtmap ['fit']) self.assertIn("Node '/fit': Missing tool: 'mkimage'", str(e.exception)) + def _CheckCapsule(self, signed_capsule=False, version_check=False): + # Firmware Management Protocol GUID used in capsule header + self.capsule_guid = "edd5cb6d2de8444cbda17194199ad92a" + # Image GUID specified in the DTS + self.image_guid = "52cfd7092007104791d108469b7fe9c8" + self.fmp_signature = "4d535331" + self.fmp_size = "10" + self.fmp_fw_version = "02" + + self.capsule_data = tools.read_file(self.capsule_fname) + + self.assertEqual(self.capsule_guid, self.capsule_data.hex()[:32]) + self.assertEqual(self.image_guid, self.capsule_data.hex()[96:128]) + + if version_check: + self.assertEqual(self.fmp_signature, self.capsule_data.hex()[184:192]) + self.assertEqual(self.fmp_size, self.capsule_data.hex()[192:194]) + self.assertEqual(self.fmp_fw_version, self.capsule_data.hex()[200:202]) + + if signed_capsule: + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[4770:4778]) + elif version_check: + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[216:224]) + else: + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[184:192]) + + def _GenCapsuleCfgPayload(self, fname, contents): + capsule_dir = '/tmp/capsules/' + pathname = os.path.join(capsule_dir, fname) + dirname = os.path.dirname(pathname) + if dirname and not os.path.exists(dirname): + os.makedirs(dirname) + + with open(pathname, 'wb') as fd: + fd.write(contents) + + def testCapsuleGen(self): + """Test generation of EFI capsule""" + self.payload_data = tools.to_bytes(TEXT_DATA) + + TestFunctional._MakeInputFile('payload.txt', self.payload_data) + + self._DoReadFile('282_capsule.dts') + + self.capsule_fname = tools.get_output_filename('test.capsule') + self.assertTrue(os.path.exists(self.capsule_fname)) + + self._CheckCapsule() + + def testSignedCapsuleGen(self): + """Test generation of EFI capsule""" + self.payload_data = tools.to_bytes(TEXT_DATA) + + TestFunctional._MakeInputFile('payload.txt', self.payload_data) + + self._DoReadFile('283_capsule_signed.dts') + + self.capsule_fname = tools.get_output_filename('test.capsule') + self.assertTrue(os.path.exists(self.capsule_fname)) + + self._CheckCapsule(signed_capsule=True) + + def testCapsuleGenCfgFile(self): + """Test generation of EFI capsule through config file""" + self.payload_data = tools.to_bytes(TEXT_DATA) + + self._GenCapsuleCfgPayload('payload.txt', self.payload_data) + + self._DoReadFile('284_capsule_conf.dts') + + self.capsule_fname = '/tmp/capsules/test.capsule' + self.assertTrue(os.path.exists(self.capsule_fname)) + + self._CheckCapsule() + + def testCapsuleGenVersionSupport(self): + """Test generation of EFI capsule with version support""" + self.payload_data = tools.to_bytes(TEXT_DATA) + TestFunctional._MakeInputFile('payload.txt', self.payload_data) + + self._DoReadFile('290_capsule_version.dts') + + self.capsule_fname = tools.get_output_filename('test.capsule') + self.assertTrue(os.path.exists(self.capsule_fname)) + + self._CheckCapsule(version_check=True) + + def testCapsuleGenKeyMissing(self): + """Test that binman errors out on missing key""" + with self.assertRaises(ValueError) as e: + self._DoReadFile('285_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('286_capsule_missing_index.dts') + + self.assertIn("mkeficapsule must be provided an 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('287_capsule_missing_guid.dts') + + self.assertIn("mkeficapsule must be provided an Image GUID", + str(e.exception)) + + def testCapsuleGenPayloadMissing(self): + """Test that binman errors out on missing input(payload)image""" + with self.assertRaises(ValueError) as e: + self._DoReadFile('288_capsule_missing_payload.dts') + + self.assertIn("mkeficapsule must be provided an input filename(payload)", + str(e.exception)) + + def testCapsuleGenCapsuleFileMissing(self): + """Test that binman errors out on missing output capsule file""" + with self.assertRaises(ValueError) as e: + self._DoReadFile('289_capsule_missing.dts') + + self.assertIn("Specify the output capsule file", + str(e.exception)) + if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/282_capsule.dts b/tools/binman/test/282_capsule.dts new file mode 100644 index 0000000000..0e7fcdf8ab --- /dev/null +++ b/tools/binman/test/282_capsule.dts @@ -0,0 +1,18 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + capsule { + image-index = <0x1>; + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; + hardware-instance = <0x0>; + filename = "payload.txt"; + capsule = "test.capsule"; + }; + }; +}; diff --git a/tools/binman/test/283_capsule_signed.dts b/tools/binman/test/283_capsule_signed.dts new file mode 100644 index 0000000000..70a6690eef --- /dev/null +++ b/tools/binman/test/283_capsule_signed.dts @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + capsule { + image-index = <0x1>; + 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 = "payload.txt"; + capsule = "test.capsule"; + }; + }; +}; diff --git a/tools/binman/test/284_capsule_conf.dts b/tools/binman/test/284_capsule_conf.dts new file mode 100644 index 0000000000..5d458a1a5a --- /dev/null +++ b/tools/binman/test/284_capsule_conf.dts @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + capsule { + cfg-file = "tools/binman/test/capsule_cfg.txt"; + }; + }; +}; diff --git a/tools/binman/test/285_capsule_missing_key.dts b/tools/binman/test/285_capsule_missing_key.dts new file mode 100644 index 0000000000..7a09bbbf7b --- /dev/null +++ b/tools/binman/test/285_capsule_missing_key.dts @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + capsule { + image-index = <0x1>; + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; + hardware-instance = <0x0>; + private-key = "tools/binman/test/key.key"; + filename = "payload.txt"; + capsule = "test.capsule"; + }; + }; +}; diff --git a/tools/binman/test/286_capsule_missing_index.dts b/tools/binman/test/286_capsule_missing_index.dts new file mode 100644 index 0000000000..dad521c0b5 --- /dev/null +++ b/tools/binman/test/286_capsule_missing_index.dts @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + capsule { + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; + hardware-instance = <0x0>; + filename = "payload.txt"; + capsule = "test.capsule"; + }; + }; +}; diff --git a/tools/binman/test/287_capsule_missing_guid.dts b/tools/binman/test/287_capsule_missing_guid.dts new file mode 100644 index 0000000000..b81aa3ecd2 --- /dev/null +++ b/tools/binman/test/287_capsule_missing_guid.dts @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + capsule { + image-index = <0x1>; + hardware-instance = <0x0>; + filename = "payload.txt"; + capsule = "test.capsule"; + }; + }; +}; diff --git a/tools/binman/test/288_capsule_missing_payload.dts b/tools/binman/test/288_capsule_missing_payload.dts new file mode 100644 index 0000000000..e702ac1a1d --- /dev/null +++ b/tools/binman/test/288_capsule_missing_payload.dts @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + capsule { + image-index = <0x1>; + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; + hardware-instance = <0x0>; + capsule = "test.capsule"; + }; + }; +}; diff --git a/tools/binman/test/289_capsule_missing.dts b/tools/binman/test/289_capsule_missing.dts new file mode 100644 index 0000000000..63b3d83370 --- /dev/null +++ b/tools/binman/test/289_capsule_missing.dts @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + capsule { + image-index = <0x1>; + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; + hardware-instance = <0x0>; + filename = "payload.txt"; + }; + }; +}; diff --git a/tools/binman/test/290_capsule_version.dts b/tools/binman/test/290_capsule_version.dts new file mode 100644 index 0000000000..b829d730f1 --- /dev/null +++ b/tools/binman/test/290_capsule_version.dts @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + capsule { + image-index = <0x1>; + fw-version = <0x2>; + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; + hardware-instance = <0x0>; + filename = "payload.txt"; + capsule = "test.capsule"; + }; + }; +}; diff --git a/tools/binman/test/capsule_cfg.txt b/tools/binman/test/capsule_cfg.txt new file mode 100644 index 0000000000..bf44a431b9 --- /dev/null +++ b/tools/binman/test/capsule_cfg.txt @@ -0,0 +1,6 @@ +{ + image-index: 1 + image-guid: 09D7CF52-0720-4710-91D1-08469B7FE9C8 + payload: /tmp/capsules/payload.txt + capsule: /tmp/capsules/test.capsule +}
Add support in binman for generating capsules. The capsule parameters can be specified either through a config file or through the capsule binman entry. Also add test cases in binman for capsule generation, and enable this testing on the sandbox_spl variant. Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> --- Changes since V3: * Add test cases for covering the various capsule generation scenarios. * Add function comments in the mkeficapsule bintool. * Fix the fetch method of the mkeficapsule bintool to enable building the tool. * Add more details about the capsule parameters in the documentation as well as the code. * Fix order of module imports, and addition of blank lines in the capsule.py file. * Use SetContents in the ObtainContents method. configs/sandbox_spl_defconfig | 1 + tools/binman/btool/mkeficapsule.py | 158 ++++++++++++++++++ tools/binman/entries.rst | 37 ++++ tools/binman/etype/capsule.py | 132 +++++++++++++++ tools/binman/ftest.py | 127 ++++++++++++++ tools/binman/test/282_capsule.dts | 18 ++ tools/binman/test/283_capsule_signed.dts | 20 +++ tools/binman/test/284_capsule_conf.dts | 14 ++ tools/binman/test/285_capsule_missing_key.dts | 19 +++ .../binman/test/286_capsule_missing_index.dts | 17 ++ .../binman/test/287_capsule_missing_guid.dts | 17 ++ .../test/288_capsule_missing_payload.dts | 17 ++ tools/binman/test/289_capsule_missing.dts | 17 ++ tools/binman/test/290_capsule_version.dts | 19 +++ tools/binman/test/capsule_cfg.txt | 6 + 15 files changed, 619 insertions(+) create mode 100644 tools/binman/btool/mkeficapsule.py create mode 100644 tools/binman/etype/capsule.py create mode 100644 tools/binman/test/282_capsule.dts create mode 100644 tools/binman/test/283_capsule_signed.dts create mode 100644 tools/binman/test/284_capsule_conf.dts create mode 100644 tools/binman/test/285_capsule_missing_key.dts create mode 100644 tools/binman/test/286_capsule_missing_index.dts create mode 100644 tools/binman/test/287_capsule_missing_guid.dts create mode 100644 tools/binman/test/288_capsule_missing_payload.dts create mode 100644 tools/binman/test/289_capsule_missing.dts create mode 100644 tools/binman/test/290_capsule_version.dts create mode 100644 tools/binman/test/capsule_cfg.txt