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

[Masahisa Kojima]

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

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

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

Thanks,
Masahisa Kojima

>
> Regards,
> Simon