Hi Sughosh,
On Thu, 3 Aug 2023 at 05:08, Sughosh Ganu sughosh.ganu@linaro.org wrote:
hi Simon,
On Wed, 2 Aug 2023 at 18:23, Simon Glass sjg@chromium.org wrote:
Hi Sughosh,
On Tue, 1 Aug 2023 at 11:41, Sughosh Ganu sughosh.ganu@linaro.org wrote:
Add support in binman for generating EFI capsules. The capsule parameters can be specified through the capsule binman entry. Also add test cases in binman for testing capsule generation.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org
Changes since V5:
- Add support for the oemflag parameter used in FWU A/B updates. This was missed in the earlier version.
- Use a single function, generate_capsule in the mkeficapsule bintool, instead of the multiple functions in earlier version.
- Remove the logic for generating capsules from config file as suggested by Simon.
- Use required_props for image index and GUID parameters.
- Use a subnode for the capsule payload instead of using a filename for the payload, as suggested by Simon.
- Add a capsule generation test with oemflag parameter being passed.
configs/sandbox_spl_defconfig | 1 +
move to a later commit
Okay. I think it'd be better to have this change before adding support for efi_capsule entry in binman.
OK, I see that it is currently only built for tools-only and a smattering of other boards. Tools should be built for all boards. I suppose for now you can create a patch to enable it for all sandbox boards, e.g. 'default y if SANDBOX' in the Kconfing, in a patch before this one.
tools/binman/btool/mkeficapsule.py | 101 +++++++++++
nit: Please split this into its own commit, with the etype in the second commit.
Okay
tools/binman/entries.rst | 64 +++++++ tools/binman/etype/efi_capsule.py | 160 ++++++++++++++++++ tools/binman/ftest.py | 122 +++++++++++++ tools/binman/test/307_capsule.dts | 21 +++ tools/binman/test/308_capsule_signed.dts | 23 +++ tools/binman/test/309_capsule_version.dts | 22 +++ tools/binman/test/310_capsule_signed_ver.dts | 24 +++ tools/binman/test/311_capsule_oemflags.dts | 22 +++ tools/binman/test/312_capsule_missing_key.dts | 22 +++ .../binman/test/313_capsule_missing_index.dts | 20 +++ .../binman/test/314_capsule_missing_guid.dts | 19 +++ .../test/315_capsule_missing_payload.dts | 17 ++ 14 files changed, 638 insertions(+) create mode 100644 tools/binman/btool/mkeficapsule.py create mode 100644 tools/binman/etype/efi_capsule.py create mode 100644 tools/binman/test/307_capsule.dts create mode 100644 tools/binman/test/308_capsule_signed.dts create mode 100644 tools/binman/test/309_capsule_version.dts create mode 100644 tools/binman/test/310_capsule_signed_ver.dts create mode 100644 tools/binman/test/311_capsule_oemflags.dts create mode 100644 tools/binman/test/312_capsule_missing_key.dts create mode 100644 tools/binman/test/313_capsule_missing_index.dts create mode 100644 tools/binman/test/314_capsule_missing_guid.dts create mode 100644 tools/binman/test/315_capsule_missing_payload.dts
diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig index 8d50162b27..65223475ab 100644 --- a/configs/sandbox_spl_defconfig +++ b/configs/sandbox_spl_defconfig @@ -249,3 +249,4 @@ CONFIG_UNIT_TEST=y CONFIG_SPL_UNIT_TEST=y CONFIG_UT_TIME=y CONFIG_UT_DM=y +CONFIG_TOOLS_MKEFICAPSULE=y
[..]
- def ReadEntries(self):
"""Read the subnode to get the payload for this capsule"""# We can have a single payload per capsuleif len(self._node.subnodes) != 1:self.Raise('The capsule entry expects a single subnode for payload')No, we mustn't restruit this. Why would we?
The mkeficapsule tool that generates the capsule expects a single image as it's input payload. Why do you see the need to support multiple entries as payload?
That's how binman works...it isn't the entry type's job to decide how the image is laid out. The user could add a section in any cases and get around any restriction you put here. But the restriction isn't necessary and is just strange.
mkeficapsule [options] <image blob> <output file>
for node in self._node.subnodes:entry = Entry.Create(self, node)entry.ReadNode()self._entries[entry.name] = entry- def _GenCapsule(self):
return self.mkeficapsule.generate_capsule(self.image_index,self.image_guid,self.hardware_instance,self.payload,self.capsule_fname,self.private_key,self.pub_key_cert,self.monotonic_count,self.fw_version,self.capoemflags)- def BuildSectionData(self, required):
if self.auth:if not os.path.isabs(self.private_key):self.private_key = tools.get_input_filename(self.private_key)if not os.path.isabs(self.pub_key_cert):self.pub_key_cert = tools.get_input_filename(self.pub_key_cert)These should be local vars, not class vars.
?
These are properties being passed by the user. Why should only these be local vars?
Not really.
self.private_key is a string, but here you turn it into an input filename. You are changing the original property. Just use a local var.
Also these are properties passed in by the user, so you should not be converting them to filenames.
These properties are actually paths to the private and public key cert files that a user will pass. In the normal usage, the user would be expected to provide the absolute path to the private and public key cert files. The above logic is needed for binman tests and out of tree builds with relative paths.
I'm fine with the logic, just don't update the property.
data, self.payload, _ = self.collect_contents_to_file(Please don't add new things to the class in the middle of the class. Doesn't pylint warn about this? This can just be a local var.
Changed this to a local var.
self._entries.values(), 'capsule_payload')outfile = self._filename if self._filename else self._node.nameself.capsule_fname = tools.get_output_filename(outfile)No, the class is for holding properties...you don't use this outside the function, so just:
[..]
- def ProcessContents(self):
ok = super().ProcessContents()data = self.BuildSectionData(True)ok2 = self.ProcessContentsUpdate(data)return ok and ok2If that is the same as the entry_Section function, you can drop it
There are differences, so this is needed.
Why is it different? Deleting the code has no effect on the tests that I can see.
[..]
payload_data = EFI_CAPSULE_DATA# Firmware Management Protocol(FMP) GUID - offset(0 - 32)self.assertEqual(FW_MGMT_GUID, data.hex()[:32])# Image GUID - offset(96 - 128)self.assertEqual(CAPSULE_IMAGE_GUID, data.hex()[96:128])if capoemflags:# OEM Flags - offset(40 - 44)self.assertEqual(oemflag, data.hex()[40:44])if signed_capsule and version_check:# FMP header signature - offset(4770 - 4778)self.assertEqual(fmp_signature, data.hex()[4770:4778])# FMP header size - offset(4778 - 4780)Can you mention where these values came from and how you created them? This all seems very brittle. Is there a tool that prints them out, or..?
Like I mentioned earlier as well, the contents of a capsule are dependent on the input parameters specified in it's generation. For e.g. the capsule generated would be different when opting for passing the version param. Similarly when generating a signed capsule -- even with a signed capsule, the contents would depend on the keys. The offsets therefore are not fixed but depend on the inputs specified. I have added comments here for better understanding. Not sure what can be done to improve this.
This is also one reason why the testing of the EFI capsule update feature should use capsules generated as part of the sandbox platform build. That makes the testing that much more robust.
We have the same problem with mkimage. Is there a 'dump_capsule' command that can output information about it? Then you could use that in this test.
self.assertEqual(fmp_size, data.hex()[4778:4780])# firmware version - offset(4786 - 4788)self.assertEqual(fmp_fw_version, data.hex()[4786:4788])# payload offset signed capsule(4802 - 4808)self.assertEqual(payload_data.hex(), data.hex()[4802:4808])elif signed_capsule:# payload offset signed capsule(4770 - 4776)self.assertEqual(payload_data.hex(), data.hex()[4770:4776])elif version_check:# FMP header signature - offset(184 - 192)self.assertEqual(fmp_signature, data.hex()[184:192])# FMP header size - offset(192 - 194)self.assertEqual(fmp_size, data.hex()[192:194])# firmware version - offset(200 - 202)self.assertEqual(fmp_fw_version, data.hex()[200:202])# payload offset for non-signed capsule with version header(216 - 222)
[..]
hardware-instance = <0x0>;blob-ext {filename = "efi_capsule_payload.bin";Do you need this filename?
Yes, this is the payload that will be used in generating the capsule.
Oh, this is so confusing. I think you can just make this a 'blob' instead of a 'blob-ext'. How about capsule_input.bin as it is shorter?
[..]
Regards, Simon