[PATCH v6 6/9] binman: capsule: Add support for generating EFI capsules
Simon Glass
sjg at chromium.org
Fri Aug 4 05:02:23 CEST 2023
Hi Sughosh,
On Thu, 3 Aug 2023 at 05:08, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> hi Simon,
>
> On Wed, 2 Aug 2023 at 18:23, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Tue, 1 Aug 2023 at 11:41, Sughosh Ganu <sughosh.ganu at 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 at 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 capsule
> > > + if 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.name
> > > + self.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 ok2
> >
> > If 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
More information about the U-Boot
mailing list