Hi Heinrich,
On Mon, 30 Sept 2024 at 06:05, Heinrich Schuchardt <xypron.g...@gmx.de> wrote:
>
> On 27.09.24 18:50, Simon Glass wrote:
> > Hi Ilias,
> >
> > On Fri, 27 Sept 2024 at 07:51, Ilias Apalodimas
> > <ilias.apalodi...@linaro.org> wrote:
> >>
> >> Hi Simon,
> >>
> >> On Fri, 27 Sept 2024 at 01:04, Simon Glass <s...@chromium.org> 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.
> >>>
> >>> Signed-off-by: Simon Glass <s...@chromium.org>
> >>> ---
> >>>
> >>> (no changes since v1)
> >>>
> >>> 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 6f6fa8d629d..41083e7c137 100644
> >>> --- a/lib/efi_loader/Kconfig
> >>> +++ b/lib/efi_loader/Kconfig
> >>> @@ -564,6 +564,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+
> >>> +/*
> >>> + * Hello world EFI application
> >>> + *
> >>> + * Copyright 2024 Google LLC
> >>> + * Written by Simon Glass <s...@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");
> >>> +
> >>> + /* exit boot services so that this part of U-Boot can be tested */
> >>> + boottime->exit_boot_services(handle, map_key);
> >>
> >> What Heinrich keeps repeating here, is that after you call EBS, boot
> >> services must not be called again.
> >>
> >>> +
> >>> + /* now exit for real */
> >>> + ret = boottime->exit(handle, ret, 0, NULL);
> >>
> >> IOW this is wrong. It might happen to work because not much has run
> >> and the memory will still be there. But you certainly should not rely
> >> on that
> >
> > Well, not much has run because it is running the test program. So we
> > know it will be OK.
> >
> > One of the challenges we have is that we need to design things for
> > testing, i.e. to make testing easy and practical. The call to exit()
> > happily returns back to sandbox and all is well. It serves the purpose
> > of the test, which is all the app is wanting to do.
> >
> > So I have to ask, what problem are you seeing?
>
> We write tests to see if our firmware conforms to the UEFI
> specification. This requires that our tests are conformant.
>
> Please, call the ResetSystem() runtime service here.
>
> I would prefer if you could at least consider suggestions.
I have not seen any suggestions about the boottime-exit() prior to
this one. Did I miss something? Anyway, I will give it a try.
Reegards,
Simon
