[PATCH v7 09/11] sandbox: capsule: Generate capsule related files through binman

[Simon Glass]

Hi Sughosh,

On Sat, 5 Aug 2023 at 13:12, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> hi Simon,
>
> On Sun, 6 Aug 2023 at 00:06, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Sat, 5 Aug 2023 at 12:01, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Sat, 5 Aug 2023 at 20:34, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > >
> > > > > The EFI capsule files can now be generated as part of u-boot
> > > > > build through binman. Add capsule entry nodes in the u-boot.dtsi for
> > > > > the sandbox architecture for generating the capsules. These capsules
> > > > > are then used for testing the EFI capsule update functionality on the
> > > > > sandbox platforms. Also add binman nodes for generating the input
> > > > > files used for these capsules.
> > > > >
> > > > > Remove the corresponding logic which was used for generation of these
> > > > > input files which is now superfluous.
> > > > >
> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > > ---
> > > > > Changes since V6:
> > > > > * Use macros defined in sandbox_efi_capsule for GUIDs and capsule
> > > > >   input filenames.
> > > > > * Generate the capsule input files through binman text entries.
> > > > >
> > > > >  arch/sandbox/dts/u-boot.dtsi                  | 363 ++++++++++++++++++
> > > > >  include/sandbox_efi_capsule.h                 |  11 +
> > > > >  test/py/tests/test_efi_capsule/conftest.py    | 168 +-------
> > > > >  test/py/tests/test_efi_capsule/signature.dts  |  10 -
> > > > >  .../tests/test_efi_capsule/uboot_bin_env.its  |  36 --
> > > > >  5 files changed, 385 insertions(+), 203 deletions(-)
> > > > >  delete mode 100644 test/py/tests/test_efi_capsule/signature.dts
> > > > >  delete mode 100644 test/py/tests/test_efi_capsule/uboot_bin_env.its
> > > >
> > > > I think you are still getting confused with using filenames when you
> > > > don't need to. See below...
> > >
> > > No, I don't think so. This is being done on purpose, since I want
> > > these files to be created. These are then copied to the efi capsule
> > > update tests' data directory, and are then used in testing the
> > > feature. Maybe if you want, I can put a comment to that effect.
> >
> > I just traced through this code. I really think this needs to be
> > simplified. You can do it in a patch at the end if you like.
> >
> > But you have the 'u-boot.bin.old' and 'Old' strings in
> > test_efi_capsule_auth2, for example.
> >
> > In the binman world you define UBOOT_BIN_IMAGE_OLD as
> > "u-boot.bin.old", then use that in the sandbox.dtsi
> >
> > Then binman creates the u-boot.bin.old file (unnecessarily in my view)
> > Then in efi_capsule_data() you copy the file to the test directory.
> >
> > So for the last step, you could just create the file again, rather
> > than copying it from the U-Boot build directory. After all, you know
> > the contents. If you like you could put the different contents as
> > binary strings in capsule_defs.py
> >
> > Then you don't need to create the files.
>
> Okay. TBH, I thought you would ask me to reuse the files created in
> binman for the tests as well, which is why I put this logic. I will
> create these files as part of the tests then.
>
> >
> > There is a lot more I can say about the EFI capsule tests. For now I
> > think we'll have to live with it creating 19 different binman images
> > on sandbox. I think we could put them in an efi_capsules subdir, but
> > that would need to be created somewhere, since binman doesn't do it. I
> > suppose we could make binman automatically create a directory if an
> > entry filename needs it?
>
> I think this can be taken up as a follow-up. I also was thinking of
> adding a flag/property to not generate all the map files. I don't
> think such a property exists currently. The map files really are not
> needed for the capsules.

Sure, but if you implemented SetImagePos() then it would work.
Something to think about when you create the tool to dump capsules.

>
> >
> > Anyway, for tests, primarily we need to split things up, so we have,
> > for example:
> >
> > process_capsule_file()
> >
> > which processes the capsule in U-Boot, e.g. using an 'efi' command,
> > then outputs what it did. Then the test can plant the capsule, run
> > that function and check that the output is as expected. This can
> > actually be a unit test, i.e. written in C.
> >
> > Most of the tests can do this. Then we can have one test that checks
> > the whole flow, but it doesn't need to be done for every case, as now.
>
> I believe even Ilias thinks that these tests should be in C. But this
> is a separate effort, not related to this series. I also have doubts
> about your observation on not using so many capsule files for tests,
> but that can be investigated separately. For now, I want to focus on
> getting these changes in, followed by the generation of capsules
> through the config file. And FWIW, I am able to use relative paths in
> the binman tests for generating and testing the capsules generated
> through the config file -- so that takes care of your objection to
> using absolute paths. I will send that series once this gets merged.

OK, yes it is best to get this series in and worry about tdy-ups after that.

Regards,
Simon