| Message ID | 20241022120032.196311-3-sjg@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | efi: Add a test for EFI bootmeth | expand |
Hi Simon, On Tue, 22 Oct 2024 at 15:00, Simon Glass <sjg@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. > > 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. Yes, I haven't had time to look at that whole discussion, but since this is a test app I don't care too much if it violates the EFI spec or not. OTOH we have lib/efi_selftest/efi_selftest_exitbootservices.c to test EBS. So why can't you just remove that call, exit normally and check the results you want? > > Signed-off-by: Simon Glass <sjg@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+ > +/* > + * Hello world EFI application > + * > + * Copyright 2024 Google LLC > + * Written by Simon Glass <sjg@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 */ Please add a comment and a print that U-Boot needs to reboot from that point onwards to continue testing Thanks /Ilias > + boottime->exit_boot_services(handle, map_key); > + > + /* now exit for real */ > + ret = boottime->exit(handle, ret, 0, NULL); > + > + /* We should never arrive here */ > + return ret; > +} > -- > 2.43.0 >
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@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@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. > + > + /* We should never arrive here */ > + return ret; After ExitBootServices() you must not return. You can just do an endless loop if ResetSystem() fails: for (;;); Best regards Heinrich > +}
On Mon, Oct 28, 2024 at 06:59:10AM +0100, Heinrich Schuchardt 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@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. We should switch to the proper one here, but there are numerous examples of this today. > > +/* > > + * Hello world EFI application > > + * > > + * Copyright 2024 Google LLC > > + * Written by Simon Glass <sjg@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. Right, but the point is to not do that and instead test what happens. I think Ilias had said we need to make some big loud print on console that you must reset the system for it to be usable afterwards, would that be enough?
On 10/28/24 16:17, Tom Rini wrote: > On Mon, Oct 28, 2024 at 06:59:10AM +0100, Heinrich Schuchardt 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@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. > > We should switch to the proper one here, but there are numerous examples > of this today. > >>> +/* >>> + * Hello world EFI application >>> + * >>> + * Copyright 2024 Google LLC >>> + * Written by Simon Glass <sjg@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. > > Right, but the point is to not do that and instead test what happens. I > think Ilias had said we need to make some big loud print on console that > you must reset the system for it to be usable afterwards, would that > be enough? > There is no point in testing what happens as this call is not allowable. Do we really need to zero out the bootservices table for people who do not read the spec to find out the hard way? Best regards Heinrich
On Mon, Oct 28, 2024 at 05:47:08PM +0100, Heinrich Schuchardt wrote: > On 10/28/24 16:17, Tom Rini wrote: > > On Mon, Oct 28, 2024 at 06:59:10AM +0100, Heinrich Schuchardt 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@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. > > > > We should switch to the proper one here, but there are numerous examples > > of this today. > > > > > > +/* > > > > + * Hello world EFI application > > > > + * > > > > + * Copyright 2024 Google LLC > > > > + * Written by Simon Glass <sjg@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. > > > > Right, but the point is to not do that and instead test what happens. I > > think Ilias had said we need to make some big loud print on console that > > you must reset the system for it to be usable afterwards, would that > > be enough? > > > > There is no point in testing what happens as this call is not allowable. > > Do we really need to zero out the bootservices table for people who do > not read the spec to find out the hard way? Then perhaps this is a case where "test causes U-Boot to reset, check for that as the result" is what the test needs to do, as it's not an arbitrary reset?
Hi Heinrich, On Mon, 28 Oct 2024 at 07:11, Heinrich Schuchardt <xypron.glpk@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@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@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? Regards, Simon
On 10/28/24 17:52, Tom Rini wrote: > On Mon, Oct 28, 2024 at 05:47:08PM +0100, Heinrich Schuchardt wrote: >> On 10/28/24 16:17, Tom Rini wrote: >>> On Mon, Oct 28, 2024 at 06:59:10AM +0100, Heinrich Schuchardt 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@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. >>> >>> We should switch to the proper one here, but there are numerous examples >>> of this today. >>> >>>>> +/* >>>>> + * Hello world EFI application >>>>> + * >>>>> + * Copyright 2024 Google LLC >>>>> + * Written by Simon Glass <sjg@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. >>> >>> Right, but the point is to not do that and instead test what happens. I >>> think Ilias had said we need to make some big loud print on console that >>> you must reset the system for it to be usable afterwards, would that >>> be enough? >>> >> >> There is no point in testing what happens as this call is not allowable. >> >> Do we really need to zero out the bootservices table for people who do >> not read the spec to find out the hard way? > > Then perhaps this is a case where "test causes U-Boot to reset, check > for that as the result" is what the test needs to do, as it's not an > arbitrary reset? > Yes, we should check that a reset occurs after ResetSystem(). Currently we are only testing that the EFI watchdog resets the system in test_efi_selftest_watchdog_reboot(). As on all non-sandbox systems all devices are removed at ExitBootServices() this is the only way forward to write a test that works on all UEFI supporting U-Boot systems. Best regards Heinrich
On 10/28/24 18:00, Simon Glass wrote: > Hi Heinrich, > > On Mon, 28 Oct 2024 at 07:11, Heinrich Schuchardt <xypron.glpk@gmx.de > <mailto:xypron.glpk@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@chromium.org <mailto:sjg@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@chromium.org <mailto:sjg@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. The test seems not to build on anything but sandbox. We should be able to test the boot methods on QEMU systems. 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. EDK II does the following in ExitBootServices(): // // Clear the non-runtime values of the EFI System Table // gDxeCoreST->BootServices = NULL; gDxeCoreST->ConIn = NULL; gDxeCoreST->ConsoleInHandle = NULL; gDxeCoreST->ConOut = NULL; gDxeCoreST->ConsoleOutHandle = NULL; gDxeCoreST->StdErr = NULL; gDxeCoreST->StandardErrorHandle = NULL; As said ExitBootServices is the point of no-return. Please, write a compliant test that runs on all UEFI architectures. Then we can review it further. Best regards Heinrich
Hi Heinrich, On Mon, 28 Oct 2024 at 19:43, Heinrich Schuchardt <xypron.glpk@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@gmx.de > > <mailto:xypron.glpk@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@chromium.org <mailto:sjg@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@chromium.org <mailto:sjg@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. > > 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. > > 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. > > EDK II does the following in ExitBootServices(): > > // > // Clear the non-runtime values of the EFI System Table > // > gDxeCoreST->BootServices = NULL; > gDxeCoreST->ConIn = NULL; > gDxeCoreST->ConsoleInHandle = NULL; > gDxeCoreST->ConOut = NULL; > gDxeCoreST->ConsoleOutHandle = NULL; > gDxeCoreST->StdErr = NULL; > gDxeCoreST->StandardErrorHandle = NULL; > > As said ExitBootServices is the point of no-return. > > Please, write a compliant test that runs on all UEFI architectures. Then > we can review it further. I am only focused on sandbox, for now. If you look at the bootflow tests, they are all for sandbox. That is enough to test the logic of bootstd. Of course, you may expand the tests to other architectures, but that is not the goal of this series. Regards, Simon
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@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@gmx.de > > > <mailto:xypron.glpk@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@chromium.org <mailto:sjg@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@chromium.org <mailto:sjg@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.
Hi Tom, On Tue, 29 Oct 2024 at 17:46, Tom Rini <trini@konsulko.com> wrote: > > 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@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@gmx.de > > > > <mailto:xypron.glpk@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@chromium.org <mailto:sjg@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@chromium.org <mailto:sjg@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. Sorry, but I cannot tell the difference. We should not have tests which need to restart, except in extreme circumstances. > > 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. Which work-around are you referring to? I discussed this somewhat with Heinrich earlier. We didn't really resolve it, but I did point out that if we want to reset/reinit the EFI system within U-Boot (as we do with driver model, devicetree and some other things) we can implement that when it is needed. > > > > 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. I'm not sure how best to attack this, even now. With the Labgrid integration (I am still hoping will land soon) I can boot into QEMU and perhaps into grub, but booting right into a distro is going to need ssh and networking (which is fine, it's just another step). Anyway, that is a bid sideways of your point. Sandbox tests are always going to be much easier to write than Python ones. They are faster to run, as well. We can't do things like 'assert_nextline()' with QEMU. Also I can't single step into QEMU with the debugger. Also with sandbox, it is possible to check the internal U-Boot-state, whereas with QEMU we have to print something on the console. So I believe that writing a QEMU test is an entirely different job from writing a sandbox test. There is something fundamental here which can be changed. Sandbox is really just better for testing U-Boot code, so long as we design the code properly for testing. Of course if we insist on not designing the code properly for testing, that erodes the advantage. But that would be the wrong direction. > > > > 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. It depends on the ordering of things in CI. From what I can tell most (or all?) of them run after this test. But if this does cause a problem with future tests that we add, we should be able to fix it easily enough. In fact it would be nice to be able to re-init EFI. Regards, Simon
On Wed, Oct 30, 2024 at 08:41:27AM +0100, Simon Glass wrote: > Hi Tom, > > On Tue, 29 Oct 2024 at 17:46, Tom Rini <trini@konsulko.com> wrote: > > > > 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@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@gmx.de > > > > > <mailto:xypron.glpk@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@chromium.org <mailto:sjg@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@chromium.org <mailto:sjg@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. > > Sorry, but I cannot tell the difference. We should not have tests > which need to restart, except in extreme circumstances. That's unfortunate then. Perhaps you'll just have to trust me that there are in fact tests where it's only valid to restart the system and not fake restart the system. > > 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. > > Which work-around are you referring to? Everything you're doing here. > I discussed this somewhat with Heinrich earlier. We didn't really > resolve it, but I did point out that if we want to reset/reinit the > EFI system within U-Boot (as we do with driver model, devicetree and > some other things) we can implement that when it is needed. That would be even more workaround. > > > > 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. > > I'm not sure how best to attack this, even now. With the Labgrid > integration (I am still hoping will land soon) I can boot into QEMU > and perhaps into grub, but booting right into a distro is going to > need ssh and networking (which is fine, it's just another step). > > Anyway, that is a bid sideways of your point. Sandbox tests are always > going to be much easier to write than Python ones. They are faster to > run, as well. We can't do things like 'assert_nextline()' with QEMU. > Also I can't single step into QEMU with the debugger. Also with > sandbox, it is possible to check the internal U-Boot-state, whereas > with QEMU we have to print something on the console. So I believe that > writing a QEMU test is an entirely different job from writing a > sandbox test. I'm not sure how this is at all relevant to the point here. If you make the test application follow the UEFI spec this is no different from the existing hello world test we have in most respects. > There is something fundamental here which can be changed. Sandbox is > really just better for testing U-Boot code, so long as we design the > code properly for testing. Of course if we insist on not designing the > code properly for testing, that erodes the advantage. But that would > be the wrong direction. To be frank, I haven't been able to run the whole sandbox test suite locally in the last 5+ years if ever, and had to throw it in to docker, and then just got tired of fighting it and only see it in CI. > > > > 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. > > It depends on the ordering of things in CI. From what I can tell most > (or all?) of them run after this test. Well, you shouldn't and I think earlier Heinrich was saying that perhaps we need to NULL out a few things more then. > But if this does cause a problem with future tests that we add, we > should be able to fix it easily enough. In fact it would be nice to be > able to re-init EFI. Sounds like a lot of work-around work just to avoid testing that we are in fact exiting services and resetting like we should.
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+ +/* + * Hello world EFI application + * + * Copyright 2024 Google LLC + * Written by Simon Glass <sjg@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); + + /* now exit for real */ + ret = boottime->exit(handle, ret, 0, NULL); + + /* We should never arrive here */ + return ret; +}
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@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