[PATCH v4 06/12] binman: capsule: Add support for generating capsules
Simon Glass
sjg at chromium.org
Wed Jul 19 03:08:20 CEST 2023
Hi Sughosh,
On Mon, 17 Jul 2023 at 04:44, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> hi Simon,
>
> On Sun, 16 Jul 2023 at 05:12, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Sat, 15 Jul 2023 at 07:46, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > >
> > > Add support in binman for generating capsules. The capsule parameters
> > > can be specified either through a config file or through the capsule
> > > binman entry. Also add test cases in binman for capsule generation,
> > > and enable this testing on the sandbox_spl variant.
> >
> > Can you use sandbox instead, or perhaps sandbox_spl? SPL is really for
> > SPL testing.
>
> Er, I am actually using the sandbox_spl variant.
>
> >
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > ---
> > > Changes since V3:
> > > * Add test cases for covering the various capsule generation
> > > scenarios.
> > > * Add function comments in the mkeficapsule bintool.
> > > * Fix the fetch method of the mkeficapsule bintool to enable building
> > > the tool.
> > > * Add more details about the capsule parameters in the documentation
> > > as well as the code.
> > > * Fix order of module imports, and addition of blank lines in the
> > > capsule.py file.
> > > * Use SetContents in the ObtainContents method.
> > >
> > > configs/sandbox_spl_defconfig | 1 +
> > > tools/binman/btool/mkeficapsule.py | 158 ++++++++++++++++++
> > > tools/binman/entries.rst | 37 ++++
> > > tools/binman/etype/capsule.py | 132 +++++++++++++++
> > > tools/binman/ftest.py | 127 ++++++++++++++
> > > tools/binman/test/282_capsule.dts | 18 ++
> > > tools/binman/test/283_capsule_signed.dts | 20 +++
> > > tools/binman/test/284_capsule_conf.dts | 14 ++
> > > tools/binman/test/285_capsule_missing_key.dts | 19 +++
> > > .../binman/test/286_capsule_missing_index.dts | 17 ++
> > > .../binman/test/287_capsule_missing_guid.dts | 17 ++
> > > .../test/288_capsule_missing_payload.dts | 17 ++
> > > tools/binman/test/289_capsule_missing.dts | 17 ++
> > > tools/binman/test/290_capsule_version.dts | 19 +++
> > > tools/binman/test/capsule_cfg.txt | 6 +
> > > 15 files changed, 619 insertions(+)
> > > create mode 100644 tools/binman/btool/mkeficapsule.py
> > > create mode 100644 tools/binman/etype/capsule.py
> > > create mode 100644 tools/binman/test/282_capsule.dts
> > > create mode 100644 tools/binman/test/283_capsule_signed.dts
> > > create mode 100644 tools/binman/test/284_capsule_conf.dts
> > > create mode 100644 tools/binman/test/285_capsule_missing_key.dts
> > > create mode 100644 tools/binman/test/286_capsule_missing_index.dts
> > > create mode 100644 tools/binman/test/287_capsule_missing_guid.dts
> > > create mode 100644 tools/binman/test/288_capsule_missing_payload.dts
> > > create mode 100644 tools/binman/test/289_capsule_missing.dts
> > > create mode 100644 tools/binman/test/290_capsule_version.dts
> > > create mode 100644 tools/binman/test/capsule_cfg.txt
> >
> > This looks pretty good to me. Some nits below
> >
> > >
> > > diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig
> > > index dd848c57c6..2fcc789347 100644
> > > --- a/configs/sandbox_spl_defconfig
> > > +++ b/configs/sandbox_spl_defconfig
> > > @@ -248,3 +248,4 @@ CONFIG_UNIT_TEST=y
> > > CONFIG_SPL_UNIT_TEST=y
> > > CONFIG_UT_TIME=y
> > > CONFIG_UT_DM=y
> > > +CONFIG_TOOLS_MKEFICAPSULE=y
> >
> > Why enabling this here? I don't think it is needed in sandbox_spl, but
> > in any case it should be in a different patch if needed.
>
> The binman tests run on the sandbox_spl variant. When running the
> capsule generation tests, the mkeficapsule tool should be present on
> the board variant no?
Can we run this on the 'sandbox' variant instead?
[..]
> >
> > > +
> > > + def ReadNode(self):
> > > + super().ReadNode()
> > > +
> > > + self.cfg_file = fdt_util.GetString(self._node, 'cfg-file')
> > > + if not self.cfg_file:
> > > + self.image_index = fdt_util.GetInt(self._node, 'image-index')
> > > + if not self.image_index:
> > > + self.Raise('mkeficapsule must be provided an Image Index')
> > > +
> > > + self.image_guid = fdt_util.GetString(self._node, 'image-type-id')
> > > + if not self.image_guid:
> > > + self.Raise('mkeficapsule must be provided an Image GUID')
> >
> > Use self.required_props = ['image-type-id', ...] in your __init__().
> > Then this is automatic
>
> I should have clarified this during the earlier version itself. So
> these parameters are mandatory only when not using the config file. In
> the scenario of generating the capsules through config files, all
> these parameters are provided through the config file. Hence these
> explicit checks.
Hmm, I think we should consider having two different etypes, then. It
seems in fact that your entry type is doing 2-3 different things?
[..]
> > What if some of the inputs are missing? Does this handle missing blobs?
>
> Any missing input parameters are checked earlier itself.
>
> > > + if self.cfg_file:
> > > + return self.mkeficapsule.capsule_cfg_file(self.cfg_file)
> > > + elif self.auth:
> > > + return self.mkeficapsule.cmdline_auth_capsule(self.image_index,
> > > + self.image_guid,
> > > + self.hardware_instance,
> > > + self.monotonic_count,
> > > + self.private_key,
> > > + self.pub_key_cert,
> > > + self.payload,
> > > + self.capsule_fname,
> > > + self.fw_version)
> > > + else:
> > > + return self.mkeficapsule.cmdline_capsule(self.image_index,
> > > + self.image_guid,
> > > + self.hardware_instance,
> > > + self.payload,
> > > + self.capsule_fname,
> > > + self.fw_version)
Here is where I wonder whether you are putting too much in a single
etype. Here there are three different cases. Should we have 3 etypes?
[..]
> >
> > We really should not have GUIDs in the code...they are a mess.
>
> You want the UEFI capsule generation to happen through binman, and not
> mention GUIDs. That ain't happening :)
I just don't want them open-coded. They are meaningless gibberish that
no one can understand. Use #define or some other way to give them a
name.
If I wrote:
writel(0x09812374, 0x8723728)
you would have the same comment.
>
> >
> > > + # Image GUID specified in the DTS
> > > + self.image_guid = "52cfd7092007104791d108469b7fe9c8"
> > > + self.fmp_signature = "4d535331"
> > > + self.fmp_size = "10"
> > > + self.fmp_fw_version = "02"
> >
> > These should really be local vars, not members.
>
> Okay
>
> >
> > > +
> > > + self.capsule_data = tools.read_file(self.capsule_fname)
> >
> > Pass the data in here and then you don't need to read the file
>
> So the file needs to be read here since the actual capsule generation
> tool(tools/mkeficapsule) does not return any capsule data. Instead,
> the data gets written to the capsule file, and the tool just returns a
> pass/fail status.
Sure, but you can read that data in the caller to this functoin.
>
> >
> > > +
> > > + self.assertEqual(self.capsule_guid, self.capsule_data.hex()[:32])
> > > + self.assertEqual(self.image_guid, self.capsule_data.hex()[96:128])
> > > +
> > > + if version_check:
> > > + self.assertEqual(self.fmp_signature, self.capsule_data.hex()[184:192])
> > > + self.assertEqual(self.fmp_size, self.capsule_data.hex()[192:194])
> > > + self.assertEqual(self.fmp_fw_version, self.capsule_data.hex()[200:202])
> > > +
> > > + if signed_capsule:
> > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[4770:4778])
> >
> > Where do these integer offsets come from? Please add a comment
>
> So, these are simply offsets in the output capsule file, which get
> impacted based on the contents being put in the capsule, like
> presence/absence of optional headers. I don't think putting a comment
> really wll add any value, because these offsets are variable.
OK then please add a comment to that effect, as well as how to figure
them out when things change.
>
> >
> > > + elif version_check:
> > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[216:224])
> > > + else:
> > > + self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[184:192])
> > > +
> > > + def _GenCapsuleCfgPayload(self, fname, contents):
> > > + capsule_dir = '/tmp/capsules/'
> >
> > You can't write to /tmp
> >
> > Please use self._indir for input files - see how other tests do it
>
> For all other tests, I am indeed using _indir and outdir. But for
> generation of capsules through a config file, we need to specify the
> location where the output capsule file will be written to. Which is
> the reason for the /tmp/capsules/. We are using this directory for
> collating all capsule related files.
Well, sorry, you can't do that. I think you should provide a relative
path, rather than absolute...that should solve the problem
[..]
> >
> > Please can you use your own test data, like EFI_DATA ? Also if you
> > declare it as a binary string you can drop the call.
> >
> > For example:
> >
> > EFI_DATA = b'efi'
>
> I don't know why text_data cannot be used, but I will add the EFI_DATA.
Well firstly it is not a 'bytes' string. Secondly you may as well have
your own as we have done with other etypes.
>
> >
> > > +
> > > + TestFunctional._MakeInputFile('payload.txt', self.payload_data)
> > > +
> > > + self._DoReadFile('282_capsule.dts')
> >
> > data = self...
>
> Please see above. We need to read the capsule file. This applies for
> all the related comments about using the data = self._DoReadFile...
That needs to be fixed, since the output file should be the capsule.
Why would the output file be anything else??
[..]
Regards,
Simon
More information about the U-Boot
mailing list