[PATCH v8 2/8] efi_loader: Add a test app
Tom Rini
trini at konsulko.com
Tue Oct 29 17:46:45 CET 2024
On Tue, Oct 29, 2024 at 04:46:16PM +0100, Simon Glass wrote:
> Hi Heinrich,
>
> On Mon, 28 Oct 2024 at 19:43, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >
> > On 10/28/24 18:00, Simon Glass wrote:
> > > Hi Heinrich,
> > >
> > > On Mon, 28 Oct 2024 at 07:11, Heinrich Schuchardt <xypron.glpk at gmx.de
> > > <mailto:xypron.glpk at gmx.de>> wrote:
> > > >
> > > > On 10/22/24 14:00, Simon Glass wrote:
> > > > > Add a simple app to use for testing. This is intended to do whatever it
> > > > > needs to for testing purposes. For now it just prints a message and
> > > > > exits boot services.
> > > > >
> > > > > There was a considerable amount of discussion about whether it is OK to
> > > > > call exit-boot-services and then return to U-Boot. This is not normally
> > > > > done in a real application, since exit-boot-services is used to
> > > > > completely disconnect from U-Boot. However, since this is a test, we
> > > > > need to check the results of running the app, so returning is
> > > necessary.
> > > > > It works correctly and it provides a nice model of how to test the EFI
> > > > > code in a simple way.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg at chromium.org <mailto:sjg at chromium.org>>
> > > > > ---
> > > > >
> > > > > (no changes since v7)
> > > > >
> > > > > Changes in v7:
> > > > > - Update commit message
> > > > >
> > > > > lib/efi_loader/Kconfig | 10 ++++++
> > > > > lib/efi_loader/Makefile | 1 +
> > > > > lib/efi_loader/testapp.c | 68 +++++++++++++++++++++++++++++++++++
> > > +++++
> > > > > 3 files changed, 79 insertions(+)
> > > > > create mode 100644 lib/efi_loader/testapp.c
> > > > >
> > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > > index 69b2c9360d8..6ced29da719 100644
> > > > > --- a/lib/efi_loader/Kconfig
> > > > > +++ b/lib/efi_loader/Kconfig
> > > > > @@ -565,6 +565,16 @@ config BOOTEFI_HELLO_COMPILE
> > > > > No additional space will be required in the resulting U-
> > > Boot binary
> > > > > when this option is enabled.
> > > > >
> > > > > +config BOOTEFI_TESTAPP_COMPILE
> > > > > + bool "Compile an EFI test app for testing"
> > > > > + default y
> > > > > + help
> > > > > + This compiles an app designed for testing. It is packed
> > > into an image
> > > > > + by the test.py testing frame in the setup_efi_image() function.
> > > > > +
> > > > > + No additional space will be required in the resulting U-
> > > Boot binary
> > > > > + when this option is enabled.
> > > > > +
> > > > > endif
> > > > >
> > > > > source "lib/efi/Kconfig"
> > > > > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> > > > > index 00d18966f9e..87131ab911d 100644
> > > > > --- a/lib/efi_loader/Makefile
> > > > > +++ b/lib/efi_loader/Makefile
> > > > > @@ -20,6 +20,7 @@ apps-$(CONFIG_EFI_LOAD_FILE2_INITRD) += initrddump
> > > > > ifeq ($(CONFIG_GENERATE_ACPI_TABLE),)
> > > > > apps-y += dtbdump
> > > > > endif
> > > > > +apps-$(CONFIG_BOOTEFI_TESTAPP_COMPILE) += testapp
> > > > >
> > > > > obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
> > > > > obj-$(CONFIG_EFI_BOOTMGR) += efi_bootmgr.o
> > > > > diff --git a/lib/efi_loader/testapp.c b/lib/efi_loader/testapp.c
> > > > > new file mode 100644
> > > > > index 00000000000..feb444c92e9
> > > > > --- /dev/null
> > > > > +++ b/lib/efi_loader/testapp.c
> > > > > @@ -0,0 +1,68 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > >
> > > > This is not a valid SPDX identifier. Please, use GPL-2.0-or-later.
> > > >
> > > > > +/*
> > > > > + * Hello world EFI application
> > > > > + *
> > > > > + * Copyright 2024 Google LLC
> > > > > + * Written by Simon Glass <sjg at chromium.org <mailto:sjg at chromium.org>>
> > > > > + *
> > > > > + * This test program is used to test the invocation of an EFI
> > > application.
> > > > > + * It writes a few messages to the console and then exits boot
> > > services
> > > > > + */
> > > > > +
> > > > > +#include <efi_api.h>
> > > > > +
> > > > > +static const efi_guid_t loaded_image_guid =
> > > EFI_LOADED_IMAGE_PROTOCOL_GUID;
> > > > > +
> > > > > +static struct efi_system_table *systable;
> > > > > +static struct efi_boot_services *boottime;
> > > > > +static struct efi_simple_text_output_protocol *con_out;
> > > > > +
> > > > > +/**
> > > > > + * efi_main() - entry point of the EFI application.
> > > > > + *
> > > > > + * @handle: handle of the loaded image
> > > > > + * @systab: system table
> > > > > + * Return: status code
> > > > > + */
> > > > > +efi_status_t EFIAPI efi_main(efi_handle_t handle,
> > > > > + struct efi_system_table *systab)
> > > > > +{
> > > > > + struct efi_loaded_image *loaded_image;
> > > > > + efi_status_t ret;
> > > > > + efi_uintn_t map_size;
> > > > > + efi_uintn_t map_key;
> > > > > + efi_uintn_t desc_size;
> > > > > + u32 desc_version;
> > > > > +
> > > > > + systable = systab;
> > > > > + boottime = systable->boottime;
> > > > > + con_out = systable->con_out;
> > > > > +
> > > > > + /* Get the loaded image protocol */
> > > > > + ret = boottime->open_protocol(handle, &loaded_image_guid,
> > > > > + (void **)&loaded_image, NULL, NULL,
> > > > > + EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> > > > > + if (ret != EFI_SUCCESS) {
> > > > > + con_out->output_string
> > > > > + (con_out, u"Cannot open loaded image
> > > protocol\r\n");
> > > > > + goto out;
> > > > > + }
> > > > > +
> > > > > + /* UEFI requires CR LF */
> > > > > + con_out->output_string(con_out, u"U-Boot test app for
> > > EFI_LOADER\r\n");
> > > > > +
> > > > > +out:
> > > > > + map_size = 0;
> > > > > + ret = boottime->get_memory_map(&map_size, NULL, &map_key,
> > > &desc_size,
> > > > > + &desc_version);
> > > > > + con_out->output_string(con_out, u"Exiting boot sevices\n");
> > > >
> > > > %s/sevices/services/g
> > > >
> > > > > +
> > > > > + /* exit boot services so that this part of U-Boot can be
> > > tested */
> > > > > + boottime->exit_boot_services(handle, map_key);
> > > > > +
> > > > > + /* now exit for real */
> > > > > + ret = boottime->exit(handle, ret, 0, NULL);
> > > >
> > > > As written before boot services are not available after
> > > > ExitBootServices(). Please, call the ResetSystem() service.
> > >
> > > I already explained this. If I call reset then U-Boot exits and the
> > > console output cannot be checked by the C test. If I tell sandbox's
> > > reset-driver to ignore the reset request, then it also doesn't come back
> > > to the test.
> > >
> > > >
> > > >
> > > > > +
> > > > > + /* We should never arrive here */
> > > > > + return ret;
> > > >
> > > > After ExitBootServices() you must not return.
> > > >
> > > > You can just do an endless loop if ResetSystem() fails:
> > > >
> > > > for (;;);
> > >
> > > No, because I need to check the output of the app. Please can you try to
> > > run this test so you can see what it is doing?
> >
> > The Python framework lets you check the output whether you are rebooting
> > or not.
>
> Yes, I complained about catching reboots at the time, but it was
> submitted against my wishes. It is not a good design to create a
> problem in a test and then work around it later. It is easier to just
> return, rather than try to catch a reset in the Python test. For one
> thing, it makes using gdb much easier to have the test be
> self-contained.
And sometimes a test needs to restart the actual system for the test.
This sounds like another case of that, and not some arbitrary bad design
restart.
To phrase that another way, you're trying to introduce workarounds in
the code to avoid having to make the test work like a real system. I do
not think that's a good idea for a test.
> > The test seems not to build on anything but sandbox. We should be able
> > to test the boot methods on QEMU systems.
>
> Most tests run only on sandbox. You are welcome to extend the tests to
> cover more architectures. However I believe the most important thing
> is to test the code itself.
We also need to write things in such a way that we don't have to start
over from scratch when testing on other emulation platforms. It's true
that it's unlikely anyone will run the full cycle of booting various off
the shelf distributions and architectures and boot media for every
change. But I've got ~2 hours between assembling a pull request and
looking at the build results and anything I can throw at a host and come
back to in that time I'll do every time. Things that are automated but
marginally longer I'll do with every tagged release.
> > In efi_exit_boot_services() we
> >
> > * call dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL)
> > * call bootm_disable_interrupts()
> > * call board_quiesce_devices()
> > * disable all EFI timers
> > * disable EFI notification
> >
> > What do you expect U-Boot to do after all of this? You cannot reasonably
> > do any follow on test without rebooting.
>
> OK, well we can always add more code later as needed. So far, this
> seems to pass CI without problems.
What EFI-based tests do you run on sandbox after this, without resetting
the system? That would perhaps help clarify the point.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20241029/29dc5bd7/attachment.sig>
More information about the U-Boot
mailing list