hi Simon,
On Wed, 26 Jul 2023 at 04:06, Simon Glass <s...@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Thu, 20 Jul 2023 at 03:20, Sughosh Ganu <sughosh.g...@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Thu, 20 Jul 2023 at 00:41, Simon Glass <s...@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Wed, 19 Jul 2023 at 02:42, Sughosh Ganu <sughosh.g...@linaro.org>
> > > wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Wed, 19 Jul 2023 at 06:41, Simon Glass <s...@chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Mon, 17 Jul 2023 at 04:44, Sughosh Ganu <sughosh.g...@linaro.org>
> > > > > wrote:
> > > > > >
> > > > > > hi Simon,
> > > > > >
> > > > > > On Sun, 16 Jul 2023 at 05:12, Simon Glass <s...@chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Sughosh,
> > > > > > >
> > > > > > > On Sat, 15 Jul 2023 at 07:46, Sughosh Ganu
> > > > > > > <sughosh.g...@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.g...@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.
> >
> > Yes, and that uses the sandbox_spl variant build for running the
> > tests. Which is why I added building the mkeficapsule tool for the
> > variant.
>
> Not really. It uses sandbox_spl to obtain the pylibfdt library, that's all.
>
> The tests should use sandbox. By enabling CONFIG_BINMAN you will get
> pylibfdt anyway.
I am not sure if I am missing something. But if I do not build the
mkeficapsule tool as part of the sandbox_spl variant, I get errors in
CI on the binman tests. And I believe that is because binman is being
run with the '--toolpath /tmp/sandbox_spl/tools', which is built for
the sandbox_spl variant. So if the mkeficapsule tool is not built for
the sandbox_spl variant, this results in the binman capsule tests to
fail in CI. I see this at least with the azure pipeline.
-sughosh
>
> >
> > >
> > > > 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.
> >
> > Can we not just build the mkeficapsule tool in the sandbox_spl
> > variant. That keeps things simple.
>
> Why is it simple? It is actually a pain, because the vast majority of
> tests only need sandbox. We use sandbox_spl for tests that do
> something in SPL and need that phase of U-Boot to perform some sort of
> test. So far as I can tell, the EFI capsule thing works entirely in
> U-Boot proper.
>
> >
> > >
> > > >
> > > > >
> > > > > [..]
> > > > >
> > > > > > >
> > > > > > > > +
> > > > > > > > + 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.
> >
> > Using a config file to specify all the parameters needed for building
> > capsule(s) is a good feature to have. It also results in generation of
> > all the needed capsules through a single invocation of the
> > mkeficapsule command, instead of multiple calls per capsule file.
>
> Does that matter? I don't really have an objection to it, except in
> that it makes the entry type confusing. Do we need to support this
> mode of operation in binman? Perhaps EFI capsule can do what ever
> other enrty type does and specify the params in the Binman node? You
> can still use the config file for other uses, but I don't see any use
> for it in binman.
>
> If you want multiple capsules, wouldn't that be done with multiple
> capsule nodes in the image definition?
>
> >
> > >
> > > >
> > > > >
> > > > > [..]
> > > > >
> > > > > > >
> > > > > > > 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?
> >
> > No confusion. Again, this does not work when generating capsules
> > through a config file.
>
> OK let's drop the config file from binman. We don't need it and I'm
> tired of the back and forth.
> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > > +
> > > > > > > > + 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.
> >
> > Instead of dropping support for generating capsules through a cfg file
> > in binman, I would say drop test coverage in binman for that case. We
> > can test this feature, that is not an issue, but you insist on using
> > the binman input and output directories which get created at runtime.
> > We cannot test the capsule generation with a cfg file with that
> > restriction.
>
> No.
>
> >
> >
> > >
> > > >
> > > > 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.
> >
> > If I could do it, I would not spend this time discussing with you.
> > Like I said above, if you insist on not using a known location for the
> > input and output files, maybe we can drop the test for cfg file --
> > this functionality does get tested as part of the capsule update
> > feature testing. I would argue that generating capsules using a cfg
> > file is beneficial and be retained.
>
> As above.
>
> >
> > >
> > > >
> > > > >
> > > > > [..]
> > > > >
> > > > > > >
> > > > > > > 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?
> >
> > In this case, it is not so. Which is why I proposed the above
> > property. What happens is that the bintool calls the actual capsule
> > generation tool, and that tool writes the capsule contents to a file
> > which is provided to the tool as a parameter. Binman does not have to
> > create the image.bin file in this case, since the mkeficapsule tool
> > has already created the capsule file -- creating the image.bin and
> > image.map is superfluous here.
>
> I think you are missing something here. The tools that binman uses are
> there to generate things that the binman description wants. The EFI
> capsule is just another one of those.
>
> We should rename 'capsule' to 'efi-capsule', I think, since there
> might be other types of capsule.
>
> In theory I could say:
>
> binman {
> u-boot-spl {
> };
> efi-capsule {
> // things we want in the capsule
> u-boot {
> }
> }
>
> and it should produce an image with SPL at the start followed by the
> capsule. The capsule is allowed to generate data, not create the whole
> image. Perhaps that is the blocker here?
>
> I see now that your capsule entry type is wonky. It needs to accept
> properties (not filenames!) and use those to package up the contents,
> which should be subnodes of itself.
>
> Probably you should look at how mkimage does it...but you essentially
> call self.collect_contents_to_file() to pick up the data. Your capsule
> should probably subclass entry_section, rather than Entry, since
> entry_Section handles some other things for you, like missing blobs.
>
> Looking at your last patch I think this is the confusion, and why this
> has been so hard for me to wrap my head around.
>
> Regards,
> Simon
