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

[Sughosh Ganu]

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.

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

-sughosh

>
> Regards,
> Simon