Hi Sughosh,
On Sat, 5 Aug 2023 at 12:42, Sughosh Ganu <sughosh.g...@linaro.org> wrote:
>
> hi Simon,
>
> On Sat, 5 Aug 2023 at 20:34, Simon Glass <s...@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.g...@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.g...@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 = 0
> > > + else:
> > > + self.auth = 1
> > > +
> > > + 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) == 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] = entry
> >
> > I 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
