[PATCH v5 4/4] test/py: efi_capsule: test for FMP versioning

[Simon Glass]

Hi Masahisa,

On Wed, 19 Apr 2023 at 23:17, Masahisa Kojima
<masahisa.kojima at linaro.org> wrote:
>
> Hi Simon,
>
> On Wed, 19 Apr 2023 at 10:47, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Masahisa,
> >
> > On Mon, 10 Apr 2023 at 03:07, Masahisa Kojima
> > <masahisa.kojima at linaro.org> wrote:
> > >
> > > This test covers FMP versioning for both raw and FIT image,
> > > and both signed and non-signed capsule update.
> > >
> > > Signed-off-by: Masahisa Kojima <masahisa.kojima at linaro.org>
> > > ---
> > > Changes in v5:
> > > - get aligned to the device tree based versioning
> > >
> > > Newly created in v4
> > >
> > >  test/py/tests/test_efi_capsule/conftest.py    |  73 +++++++
> > >  .../test_capsule_firmware_fit.py              | 187 ++++++++++++++++
> > >  .../test_capsule_firmware_raw.py              | 201 ++++++++++++++++++
> > >  .../test_capsule_firmware_signed_fit.py       | 165 ++++++++++++++
> > >  .../test_capsule_firmware_signed_raw.py       | 169 +++++++++++++++
> > >  test/py/tests/test_efi_capsule/version.dts    |  27 +++
> > >  6 files changed, 822 insertions(+)
> > >  create mode 100644 test/py/tests/test_efi_capsule/version.dts
> >
> > I hate to say this, but we must fix the reboot stuff here before
> > adding more code.
>
> I read the previous discussion.
> https://lore.kernel.org/u-boot/164388018493.446835.11931101380744085380.stgit@localhost/
>
> >
> > The test needs to tell U-Boot to do the update (e.g. via a command),
> > then continue. It should not reboot sandbox.I have said this before
> > but the feedback was not understood or taken. If you need help
> > designing this, please let me know. In fact I am happy to create a
> > patch for one of the tests if that is what it takes to convey this.
>
> Current efi capsule update test tries to verify the following behavior
> stated in the UEFI spec v2.10, so the reboot is required to trigger
> the capsule update.
>
> === quote from UEFI v2.10 P.243
> 8.5.5 Delivery of Capsules via file on Mass Storage Device
>
> As an alternative to the UpdateCapsule() runtime API, capsules of any
> type supported by platform may also be delivered to firmware via a
> file within the EFI system partition on the mass storage device
> targeted for boot. Capsules staged using this method are processed on
> the next system restart. This method is only available when booting
> from mass storage devices which are formatted with GPT and contain an
> EFI System Partition in the device image. System firmware will search
> for capsule when EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED
> bit in OsIndications is set as described in Exchanging information
> between the OS and Firmware.
> ===

Let's agree for the moment that the spec actually requires a reboot to
initial the update. For a test, we can split it into three parts:

- setting up the update
- triggering the update
- processing the update

We can write separate tests for each of these:

- test that the update is set up correctly
- test that the update is triggered on a reboot, if one is present
- test that the update is processed correctly

 There is no need to bring them all together.

>
> >
> > Also, while it is normal to have a fair bit of duplication in tests,
> > just to make them easier to read, it does make them hard to m
> aintain,
> > and the duplication here seems very large.
> >
> > We have the same code repeated but with different code style or
> > indentation. Sorry, but this is not setting us up for the future, in
> > terms of maintaining this code. Just search for 'env set -e -nv -bs
> > -rt OsIndications =0x0000000000000004' for example.
> >
> > The common code needs to go in functions in a separate Python file, as
> > we have done for other tests. The goal should be to use the same code
> > for the same functions. Sure there will be some duplication, but it is
> > too much.
> >
> > These fixes should happen before we accept any more non=trivial
> > patches to this code.
>
> I agree, I will try to create common functions to reduce code duplication.

OK great.

>
> >
> > BTW I do wonder whether the test would be better written mostly in C,
> > since from what I can see, it mostly just runs commands. You can still
> > have a Python wrapper - see for example how test_vbe works. It uses
> > 'ut xxx -f' to execute the C part of a Python test. You can do setup
> > in Python, then run the C test. What do you think?
>
> It is just my opinion, if the test requires some setup in Python, I like Python
> to write the test cases as far as tests can be implemented in Python,
> because we can maintain the test code in one place.

Actually almost all the test code can go in C. See here for the trade-offs [1].

Regards,
Simon

[1] https://u-boot.readthedocs.io/en/latest/develop/tests_writing.html