Hi Sughosh,
On Sat, 5 Aug 2023 at 12:42, Sughosh Ganu sughosh.ganu@linaro.org wrote:
hi Simon,
On Sat, 5 Aug 2023 at 20:34, Simon Glass sjg@chromium.org wrote:
Hi Sughosh,
On Sat, 5 Aug 2023 at 05:35, 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 V6:
- Add macros for the GUID strings in sandbox_efi_capsule.h
- Highlight that the private and public keys are mandatory for capsule signing.
- Add a URL link to the UEFI spec, as used in the rst files.
- Use local vars for private and public keys in BuildSectionData()
- Use local vars for input payload and capsule filenames in BuildSectionData().
- Drop the ProcessContents() and SetImagePos() as the superclass functions suffice.
- Use GUID macro names in the capsule test dts files.
- Rename efi_capsule_payload.bin to capsule_input.bin.
include/sandbox_efi_capsule.h | 14 ++
Please move this file to a later patch - see below.
The idea was to also be able to run the binman capsule tests once this patch was applied. If we are to move this to a separate patch, it should be the one before this patch. But I guess based on your other reply, this might not be needed after all.
Yes, it should not be needed if we name the test GUIDs. Remember that binman is a standalone tool so cannot reference files outside tools/...although there is no test for that so some things may have crept in.
Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.
Umm, I am not too sure. Maybe we can take a call at a later point if there are too many files that start cropping up.
OK
tools/binman/entries.rst | 62 ++++++++ tools/binman/etype/efi_capsule.py | 143 ++++++++++++++++++ tools/binman/ftest.py | 121 +++++++++++++++ tools/binman/test/307_capsule.dts | 23 +++ tools/binman/test/308_capsule_signed.dts | 25 +++ tools/binman/test/309_capsule_version.dts | 24 +++ tools/binman/test/310_capsule_signed_ver.dts | 26 ++++ tools/binman/test/311_capsule_oemflags.dts | 24 +++ tools/binman/test/312_capsule_missing_key.dts | 24 +++ .../binman/test/313_capsule_missing_index.dts | 22 +++ .../binman/test/314_capsule_missing_guid.dts | 19 +++ .../test/315_capsule_missing_payload.dts | 19 +++ 13 files changed, 546 insertions(+) create mode 100644 include/sandbox_efi_capsule.h 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
[..]
- def ReadNode(self):
self.ReadEntries()super().ReadNode()I believe those two lines should be swapped.
Again, like my earlier code for ProcessContents() and SetImagePos(), which was taken from mkimage.py as reference, this code is on similar lines to what is in intel_ifwi.py. Both these files are authored by you, so I took this as reference, especially mkimage.py.
OK, then take a look at mkimage.py and follow that. Yes intel_ifwi is around the wrong way. Although these days ReadEntries() is called automatically from entry_Section so you don't need to call it here.
self.image_index = fdt_util.GetInt(self._node, 'image-index')self.image_guid = fdt_util.GetString(self._node, 'image-type-id')self.fw_version = fdt_util.GetInt(self._node, 'fw-version')self.hardware_instance = fdt_util.GetInt(self._node, 'hardware-instance')self.monotonic_count = fdt_util.GetInt(self._node, 'monotonic-count')self.oem_flags = fdt_util.GetInt(self._node, 'oem-flags')self.private_key = fdt_util.GetString(self._node, 'private-key')self.public_key_cert = fdt_util.GetString(self._node, 'public-key-cert')if ((self.private_key and not self.public_key_cert) or (self.public_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.public_key_cert):self.auth = 0else:self.auth = 1- 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) == 0:self.Raise('The capsule entry expects at least one subnode for payload')Still need to drop this
? Should we not check if the input payload is missing? We cannot call the mkeficapsule tool without an input image(binary).
Why not?
for node in self._node.subnodes:entry = Entry.Create(self, node)entry.ReadNode()self._entries[entry.name] = entryI think you can drop this method, since it should be the same as entry_Sectoin ?
Will check, but again, referenced from mkimage.py.
That one is special since it has to deal with a special 'imagename' node.
Regards, Simon