[PATCH v4 06/12] binman: capsule: Add support for generating capsules
Simon Glass
sjg at chromium.org
Wed Jul 19 21:11:33 CEST 2023
Hi Sughosh,
On Wed, 19 Jul 2023 at 02:42, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> hi Simon,
>
> On Wed, 19 Jul 2023 at 06:41, Simon Glass <sjg at chromium.org> wrote:
> >
> > 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?
>
> The capsule tests can be run on sandbox. But the change in the
> sandbox_spl_defconfig is for adding support for the capsule tests
> which are run as part of the binman test suite in CI. So it would be a
The binman tests are run separately, in 'Run binman, buildman, dtoc,
Kconfig and patman testsuites' in gitlab.
> question of whether the rest of the binman tests in CI can be run on
> the sandbox variant. Or are there any specific set of tests which need
> to be run on the sandbox_spl variant?
Just the ones that need SPL. If you type 'make qcheck' you will see
most/all of the tests, including the sandbox_spl ones.
>
> >
> > [..]
> >
> > > >
> > > > > +
> > > > > + 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?
>
> I am wondering if having two different etypes might get confusing for the users.
Maybe, but I'm already confused.
>
> >
> > [..]
> >
> > > > 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?
>
> Basically the result in all the three cases is the same -- generation
> of a capsule file. Just that the input parameters being passed for
> generation are slightly different. There can be multiple entry types
> for these, but like I mentioned above, would having multiple entry
> types not be confusing for the users?
So can we drop the use of a cfg file? What is the difference between
the second two? If you had added comments I would not have had to ask.
>
> >
> > [..]
> >
> > > >
> > > > 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.
>
> Yes, I will move the GUIDS under a variable.
>
> >
> > 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.
>
> Yes, the data can be read in the caller, but that would mean
> duplicating the read in four functions. Instead the file read is
> happening in one place where the data is being used.
What do you mean? The data is returned from the call to
self._DoReadFile(), the so pattern is:
data = self._DoReadFile(...)
You should be able to see that throughout ftest.py
What exactly is the confusion here?
>
> >
> > >
> > > >
> > > > > +
> > > > > + 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.
>
> Okay
>
> >
> > >
> > > >
> > > > > + 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
>
> Using absolute paths is only being done in the case of testing capsule
> generation through config file -- all the rest of the test cases are
> using relative paths for the input file and the capsule file. For the
> generation of capsules through the config file, the paths either need
> to be absolute, or relative to the directory from which the
> mkeficapsule tool is being invoked. And I believe that the binman
> test is creating temporary directories at runtime for input and output
> files.
OK so let's drop the cfg file stuff. It doesn't seem to be needed.
>
> What is the issue you see in using the /tmp/capsules/ path for
> generation of capsules. I see directories being created with relevant
> files under /opt for other tests. If you would want the capsules
> directory to be in some other location, I can change that.
Binman tests should use the input and output directories, so please
figure out how to do this. If you drop the cfg file then it should be
easy.
>
> >
> > [..]
> >
> > > >
> > > > 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.
>
> Okay
>
> >
> > >
> > > >
> > > > > +
> > > > > + 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??
>
> The output file is indeed a capsule, but that is being generated by
> the capsule generation tool which gets called from the bintool. So the
> mkeficapsule tool that the bintool calls results in generation of the
> capsule file. In that way, this does not map directly to the concept
> of an image in binman. And I was wondering if I should introduce a new
> property like 'dummy-image', so that the resultant <image>.bin and
> <image>.map files are not created -- they really are not needed in the
> case of capsule generation. Would you be fine with an introduction of
> such a property?
Eek, no.
I thought the output file from binman (image.bin in most tests) was an
EFI capsule. Is that not the case? If not, what exactly is it?
Regards,
Simon
More information about the U-Boot
mailing list