[PATCH v6 6/9] binman: capsule: Add support for generating EFI capsules

[Simon Glass]

Hi Sughosh,

On Fri, 4 Aug 2023 at 00:44, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> 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 +
> > > >

[..]

> > [..]
> >
> > > > > +    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

There is a single input, since binman collects everything together.

> 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.

Is there some strange magic in the tool? What does it matter where the
data come from?

I really don't understand what breaks if the user adds two entries
here. We should not have this limitation.

[..]

> >
> > [..]
> >
> > > > > +        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.

OK, I think that is a good path.

>
> >
> > >
> > > >
> > > > > +            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.

Well making it blob-ext was suggesting there was some external thing
that might not be available. If you change it to 'blob' it is easier.

Yes 'input' is better as 'payload' sounds like it is the capsule update file.

Regards,
Smion