[PATCH v6 6/9] binman: capsule: Add support for generating EFI capsules
Sughosh Ganu
sughosh.ganu at linaro.org
Fri Aug 4 08:43:53 CEST 2023
hi Simon,
On Fri, 4 Aug 2023 at 08:32, Simon Glass <sjg at chromium.org> wrote:
>
> 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.
Will do.
>
> >
> >
> > >
> > > > 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.
The idea here is that if a user puts two entries, there is no clarity
on how that is to be handled, since the mkeficapsule tool expects a
single input image(binary in binman jargon). I am aware of the user
using section type to club multiple images, and I think that is
absolutely fine, since we are then left with one final binary to
present to the mkeficapsule tool. In fact, I think if the user wants a
combination of multiple binaries, they can use a section entry. In
fact a user can even use the recently added templates in binman to add
stuff to the input binary, but the final binary to be passed to the
mkeficapsule tool should be one.
>
> >
> > 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.
Okay, understood.
>
> >
> > > 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.
Understood
>
> >
> > >
> > > > + 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.
Umm, let me check this, but a cursory glance showed differences. This
is in fact taken from the mkimage.py entry.
>
> [..]
>
> > > > + 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.
We currently do not have support in the mkeficapsule tool to dump the
capsule contents. I can take a stab at it later. I would say, for now,
let's get this in. Once I have made changes to the capsule generation
tool, I will come back and change it.
>
> >
> > >
> > > > + 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?
Heh, not sure what is confusing. This is a blob which is specifying
the name of the image(binary) which goes into the capsule. If you
think the name is rather long, I will make it shorter as per your
suggestion.
-sughosh
More information about the U-Boot
mailing list