[PATCH] test/py: efi_capsule: Handle expected reset after capsule on disk

[Simon Glass]

Hi Masami,

On Tue, 15 Mar 2022 at 02:36, Masami Hiramatsu
<masami.hiramatsu at linaro.org> wrote:
>
> Hi Simon,
>
> 2022年3月15日(火) 14:04 Simon Glass <sjg at chromium.org>:
> >
> > Hi Masami,
> >
> > On Mon, 14 Mar 2022 at 18:40, Masami Hiramatsu
> > <masami.hiramatsu at linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > 2022年3月15日(火) 3:24 Simon Glass <sjg at chromium.org>:
> > > >
> > > > > > OK, well 'reset by a user' presumably starts the board up and then
> > > > > > runs some code to do the update in U-Boot? Is that right? If so, we
> > > > > > just need to trigger that update from the test. We don't need to test
> > > > > > the actual reset, at least not with sandbox. As I said, we need to
> > > > > > write the code so that it is easy to test.
> > > > >
> > > > > Actually, we already have that command, "efidebug capsule disk-update"
> > > > > which kicks the capsule update code even without the 'reset by a
> > > > > user'. So we can just kick this command for checking whether the
> > > > > U-Boot UEFI code correctly find the capsule file from ESP which
> > > > > specified by UEFI vars.
> > > > >
> > > > > However, the 'capsule update on-disk' feature is also expected (and
> > > > > defined in the spec?) to run when the UEFI subsystem is initialized.
> > > > > This behavior will not be tested if we skip the 'reset by a user'. I
> > > > > guess Takahiro's current test case tries to check it.
> > > >
> > > > The 'UEFI subsystem is intialised' is a problem, actually, since if it
> > > > were better integrated into driver model, it would not have separate
> > > > structures or they would be present and enabled when driver model is.
> > > > I hope that it can be fixed and Takahiro's series is a start in that
> > > > direction.
> > >
> > > OK.
> > >
> > > > But as to a test that an update is called when UEFI starts, that seems
> > > > like a single line of code. Sure it is nice to test it, but it is much
> > > > more important to test the installation of the update and the
> > > > execution of the update. I suppose another way to test that is  to
> > > > shut down the UEFI subsystem and start it up?
> > >
> > > Yes, currently we call do_reset() after install the capsule file.
> > > (This reset can be avoided if we replace it with
> > > sysreset_walk_halt(SYSRESET_COLD) as you said, right?)
> > >
> > > Here is how I tested it on my machine;
> > >
> > > > usb start
> > > > fatload usb 0 $kernel_addr_r test.cap
> > > > fatwrite mmc 0 $fileaddr EFI/UpdateCapsule/test.cap $filesize
> > > > efidebug capsule disk-update
> > > (run install process and reboot the machine)
> > >
> > > So, if we can avoid the last reset, we can test the below without
> > > reset on sandbox (depends on scenarios).
> > > - confirm that the capsule update on disk can find the capsule file
> > > from ESP specified by the BOOTXXXX EFI variable.
> > > - confirm that the capsule update on disk writes the firmware
> > > correctly to the storage which specified by DFU.
> > > - confirm that the capsule update on disk success if the capsule image
> > > type is supported.
> > > - confirm that the capsule update on disk fails if the capsule image
> > > type is not supported.
> > > - confirm that the capsule update on disk will reboot after update
> > > even if the update is failed.
> > >
> > > The only spec we can not test is
> > > - confirm that the capsule update on disk is kicked when the UEFI is
> > > initialized.
> >
> > Even that could be tested, by installing an update and then initing UEFI?
>
> yeah, if the UEFI is not initialized yet, we can run some UEFI related
> command (e.g. printenv -e) instead of efidebug capsule... to execute
> the capsule update on disk.
> But anyway, this is only available at the first time. We need a way to
> reset UEFI subsystem without system reset.

Yes. It is certainly possible, but I'm not sure how easy it is.
Perhaps just drop all the EFI data structures and run the EFI init
again? We have something similar with driver model. See
dm_test_pre_run()

>
>
> > > >
> > > > Anyway we should design subsystems so they are easy to test.
> > >
> > > Here I guess you mean the unit test, not system test, am I correct?
> >
> > Yes. Easy testing is so important for developer productivity and
> > happiness. It is fine to have large system/functional tests as a fall
> > back or catch-all, but they tend to test the happy path only. When
> > they fail, they are hard to debug because they cover such as large
> > area of the code and they often have complex setup requirements so are
> > hard to run manually.
> >
> > My hope is that all the functionality should be covered by unit tests
> > or integration tests, so that system/functional almost never fail.
>
> My another question is how small is the granularity of the unit test.
> As I showed, the UEFI capsule update needs to prepare a capsule file
> installed in the storage.
> That seems to be very system-level. But you think that is still be a unit test?
> (I expected that the 'Unit test' is something like KUnit in Linux)

Well I am using your terminology here. Technically many of the U-Boot
tests (executed by 'ut' command) are not really unit tests. They bring
in a lot of code and run one test case using it.

For example, one of the tests brings up the USB subsystem, including a
fake USB stick, then checks it can read data from the stick, using the
USB stack.

Another one writes some things to the emulated display and then checks
that the correct pixels are there.

Perhaps a better name would be integration test. But the point is that
we can run these tests very, very quickly and (setup aside) without
outside influence, or without restarting the executable, etc.

> >
> > >
> > > >
> > > > >
> > > > > > > Masami's patch (this series) fixes issues around those two resets
> > > > > > > in pytest.
> > > > > >
> > > > > > Yes and that is the problem I have, at least on sandbox.
> > > > >
> > > > > So If I I call sysreset_walk_halt(SYSRESET_COLD) after capsule update,
> > > > > it could help?
> > > >
> > > > Yes that can help, because sandbox can detect that and turn it into a nop.
> > >
> > > OK, let me submit a patch to update it.
> >
> > OK thank you.

Regards,
SImon