[PATCH v6 08/12] efi_loader: Disable ANSI output for tests
We don't want ANSI characters written in tests since it is a pain to check the output with ut_assert_nextline() et al.
Provide a way to tests to request that ANSI characters not be sent.
Add a proper function comment while we are here, to encourage others.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
include/efi_loader.h | 21 ++++++++++++++++++++- lib/efi_loader/efi_console.c | 26 +++++++++++++++++--------- 2 files changed, 37 insertions(+), 10 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index f84852e384f..82b90ee0f1d 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -531,8 +531,27 @@ efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index); efi_status_t efi_bootmgr_run(void *fdt); /* search the boot option index in BootOrder */ bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *index); -/* Set up console modes */ + +/** + * efi_setup_console_size() - update the mode table. + * + * By default the only mode available is 80x25. If the console has at least 50 + * lines, enable mode 80x50. If we can query the console size and it is neither + * 80x25 nor 80x50, set it as an additional mode. + */ void efi_setup_console_size(void); + +/** + * efi_console_set_ansi() - Set whether ANSI characters should be emitted + * + * These characters mess up tests which use ut_assert_nextline(). Call this + * function to tell efi_loader not to emit these characters when starting up the + * terminal + * + * @allow_ansi: Allow emitting ANSI characters + */ +void efi_console_set_ansi(bool allow_ansi); + /* Set up load options from environment variable */ efi_status_t efi_env_set_load_options(efi_handle_t handle, const char *env_var, u16 **load_options); diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index cea50c748aa..569fc9199bc 100644 --- a/lib/efi_loader/efi_console.c +++ b/lib/efi_loader/efi_console.c @@ -30,6 +30,17 @@ struct cout_mode {
__maybe_unused static struct efi_object uart_obj;
+/* + * suppress emission of ANSI codes for use by unit tests. Leave it as 0 for the + * default behaviour + */ +static bool no_ansi; + +void efi_console_set_ansi(bool allow_ansi) +{ + no_ansi = !allow_ansi; +} + static struct cout_mode efi_cout_modes[] = { /* EFI Mode 0 is 80x25 and always present */ { @@ -348,13 +359,6 @@ static int __maybe_unused query_vidconsole(int *rows, int *cols) return 0; }
-/** - * efi_setup_console_size() - update the mode table. - * - * By default the only mode available is 80x25. If the console has at least 50 - * lines, enable mode 80x50. If we can query the console size and it is neither - * 80x25 nor 80x50, set it as an additional mode. - */ void efi_setup_console_size(void) { int rows = 25, cols = 80; @@ -362,8 +366,12 @@ void efi_setup_console_size(void)
if (IS_ENABLED(CONFIG_VIDEO)) ret = query_vidconsole(&rows, &cols); - if (ret) - ret = query_console_serial(&rows, &cols); + if (ret) { + if (no_ansi) + ret = 0; + else + ret = query_console_serial(&rows, &cols); + } if (ret) return;
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 sjg@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 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; +}
Hi Simon,
On Fri, 27 Sept 2024 at 01:04, 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.
Signed-off-by: Simon Glass sjg@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 yhelpThis compiles an app designed for testing. It is packed into an imageby the test.py testing frame in the setup_efi_image() function.No additional space will be required in the resulting U-Boot binarywhen 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);
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
Thanks
/Ilias
/* We should never arrive here */return ret;+}
2.43.0
Hi Ilias,
On Fri, 27 Sept 2024 at 07:51, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Fri, 27 Sept 2024 at 01:04, 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.
Signed-off-by: Simon Glass sjg@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 yhelpThis compiles an app designed for testing. It is packed into an imageby the test.py testing frame in the setup_efi_image() function.No additional space will be required in the resulting U-Boot binarywhen 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);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?
Thanks
/Ilias
/* We should never arrive here */return ret;+}
2.43.0
Regards, SImon
On 27.09.24 18:50, Simon Glass wrote:
Hi Ilias,
On Fri, 27 Sept 2024 at 07:51, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Fri, 27 Sept 2024 at 01:04, 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.
Signed-off-by: Simon Glass sjg@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 yhelpThis compiles an app designed for testing. It is packed into an imageby the test.py testing frame in the setup_efi_image() function.No additional space will be required in the resulting U-Boot binarywhen 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);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.
Best regards
Heinrich
Thanks
/Ilias
/* We should never arrive here */return ret;+}
2.43.0
Regards, SImon
Hi Heinrich,
On Mon, 30 Sept 2024 at 06:05, Heinrich Schuchardt xypron.glpk@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.apalodimas@linaro.org wrote:
Hi Simon,
On Fri, 27 Sept 2024 at 01:04, 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.
Signed-off-by: Simon Glass sjg@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 yhelpThis compiles an app designed for testing. It is packed into an imageby the test.py testing frame in the setup_efi_image() function.No additional space will be required in the resulting U-Boot binarywhen 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);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
This is not implemented so cannot actually be used to read blocks. Disable it until it is implemented, to avoid causing a hang with EFI, which probes every available BLK device.
Signed-off-by: Simon Glass sjg@chromium.org Issue: https://source.denx.de/u-boot/u-boot/-/issues/37 ---
Changes in v6: - Drop the patch to disable sandbox virtio blk with EFI - Add new patch to disable the sandbox virtio blk device
arch/Kconfig | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/Kconfig b/arch/Kconfig index 8f1f4667012..c39efb4d0a2 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -212,7 +212,8 @@ config SANDBOX imply VIRTIO_MMIO imply VIRTIO_PCI imply VIRTIO_SANDBOX - imply VIRTIO_BLK + # Re-enable this when fully implemented + # imply VIRTIO_BLK imply VIRTIO_NET imply DM_SOUND imply PCI_SANDBOX_EP
On Fri, Sep 27, 2024 at 12:02:21AM +0200, Simon Glass wrote:
This is not implemented so cannot actually be used to read blocks. Disable it until it is implemented, to avoid causing a hang with EFI, which probes every available BLK device.
Signed-off-by: Simon Glass sjg@chromium.org Issue: https://source.denx.de/u-boot/u-boot/-/issues/37
Reviewed-by: Tom Rini trini@konsulko.com
Create a new disk for use with tests, which contains the new 'testapp' EFI app specifically intended for testing the EFI loader.
Attach it to the USB device, since most testing is currently done with mmc.
Initially this image will be used to test the EFI bootmeth.
Fix a stale comment in prep_mmc_bootdev() while we are here.
For now this uses sudo and a compressed fallback file, like all the other bootstd tests. Once this series is in, the patch which moves this to use user-space tools will be cleaned up and re-submitted.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
arch/sandbox/dts/test.dts | 2 +- test/boot/bootdev.c | 18 +++++++++- test/boot/bootflow.c | 2 +- test/py/tests/bootstd/flash1.img.xz | Bin 0 -> 5016 bytes test/py/tests/test_ut.py | 52 ++++++++++++++++++++++++---- 5 files changed, 65 insertions(+), 9 deletions(-) create mode 100644 test/py/tests/bootstd/flash1.img.xz
diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index 8412506c17a..2fc5c181a3b 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -1507,7 +1507,7 @@ flash-stick@1 { reg = <1>; compatible = "sandbox,usb-flash"; - sandbox,filepath = "testflash1.bin"; + sandbox,filepath = "flash1.img"; };
flash-stick@2 { diff --git a/test/boot/bootdev.c b/test/boot/bootdev.c index c635d06ec25..269bcd693a6 100644 --- a/test/boot/bootdev.c +++ b/test/boot/bootdev.c @@ -212,6 +212,10 @@ static int bootdev_test_order(struct unit_test_state *uts) /* Use the environment variable to override it */ ut_assertok(env_set("boot_targets", "mmc1 mmc2 usb")); ut_assertok(bootflow_scan_first(NULL, NULL, &iter, 0, &bflow)); + + /* get the usb device which has a backing file (flash1.img) */ + ut_asserteq(0, bootflow_scan_next(&iter, &bflow)); + ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow)); ut_asserteq(5, iter.num_devs); ut_asserteq_str("mmc1.bootdev", iter.dev_used[0]->name); @@ -251,7 +255,11 @@ static int bootdev_test_order(struct unit_test_state *uts) ut_assertok(bootflow_scan_first(NULL, NULL, &iter, 0, &bflow)); ut_asserteq(2, iter.num_devs);
- /* Now scan past mmc1 and make sure that the 3 USB devices show up */ + /* + * Now scan past mmc1 and make sure that the 3 USB devices show up. The + * first one has a backing file so returns success + */ + ut_asserteq(0, bootflow_scan_next(&iter, &bflow)); ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow)); ut_asserteq(6, iter.num_devs); ut_asserteq_str("mmc2.bootdev", iter.dev_used[0]->name); @@ -313,6 +321,10 @@ static int bootdev_test_prio(struct unit_test_state *uts)
/* 3 MMC and 3 USB bootdevs: MMC should come before USB */ ut_assertok(bootflow_scan_first(NULL, NULL, &iter, 0, &bflow)); + + /* get the usb device which has a backing file (flash1.img) */ + ut_asserteq(0, bootflow_scan_next(&iter, &bflow)); + ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow)); ut_asserteq(6, iter.num_devs); ut_asserteq_str("mmc2.bootdev", iter.dev_used[0]->name); @@ -330,6 +342,10 @@ static int bootdev_test_prio(struct unit_test_state *uts) bootflow_iter_uninit(&iter); ut_assertok(bootflow_scan_first(NULL, NULL, &iter, BOOTFLOWIF_HUNT, &bflow)); + + /* get the usb device which has a backing file (flash1.img) */ + ut_asserteq(0, bootflow_scan_next(&iter, &bflow)); + ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow)); ut_asserteq(7, iter.num_devs); ut_asserteq_str("usb_mass_storage.lun0.bootdev", diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index 6ad63afe90a..e05103188b4 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -525,7 +525,7 @@ static int prep_mmc_bootdev(struct unit_test_state *uts, const char *mmc_dev,
order[2] = mmc_dev;
- /* Enable the mmc4 node since we need a second bootflow */ + /* Enable the requested mmc node since we need a second bootflow */ root = oftree_root(oftree_default()); node = ofnode_find_subnode(root, mmc_dev); ut_assert(ofnode_valid(node)); diff --git a/test/py/tests/bootstd/flash1.img.xz b/test/py/tests/bootstd/flash1.img.xz new file mode 100644 index 0000000000000000000000000000000000000000..9148b91a20c1f0acb54449b124d1408961e92507 GIT binary patch literal 5016 zcmeI0cTf{r8pRVz9+4n5AV?_EgQ1BO;n5K(kx-;pUwkwL0tBUqlz==yF+ikBDAExL zQWT{~2Sq?y06~gW5eP+U$nMPkwKMCEzInSlv*Z7F=Fa`i`OZ1tr78#8*Z}|x3nSGR z=>Wn&ZU6ufAUmH=qlqxW9033y>XG5m*pL}cy$rvQVYbMeTPGAi$9qqfMnd47Jp+<S zIxLcX-gdy?;z*~}QgHUO>b8@aTLW9^ZgBe#gV-u~nri0|VMr5lA6KhSV}ds3x_d!R zK70s*W-GN^=;}ze2DKy`8Bp8a3s-y^i!J>$OdpXOt?{Uc-H+4p?j<BRmf`kr@RkOb z#nr%a+fVcMj7<BT<4=wb&7}i+1?+`%_9rcV-IGFt)xzJvuYasj+h}W$aV}-mF#cWV z0q02?Tk;;}mhB#MFO491k+tL~bm}lePR6P^F?uU1qWM~zn8n@>UP+#@pmNRUwJ*{D zGJt0<ec}SEe@BuyWsjlSn3!f77C4m?PYr2;FS!1eCM{1>cjopJaH2B8t_BV*T|(V? zQE{)kaF4zAwh?({NItl!v80%xP5j+-km~m0FjHOI&3eSrr0=y}ILxn<Dj+V+FtA;? zkfwXZQuFzB-RUPwyo#9fJZ3vFLqjRnRXx{Cjj$``T7)LJeniuGCW|XG2Di@RTK-7v z%@LmQG%VY9Q*iQk{IX5!h|sU6x5>&A^gJ={@<k08%TNsA;LO8|NDKjWvI9|*L4$MB z+7n%gV=Mx1tAg1*_I$;8%e5KZC}1lz<@sfc9Rl028JFGGi3IeoQhBA1Rw!e4KbgZ> zcWW%yKY}Nl<I~z}t_$|PJU0<-g!)ivhK&n#A9YhnK-3N*Q`%=WA+6TycBy`{rYBep zuUe97ouG<2ucIu+XU;2LR8g8yklNz3;<k<<Au8XakMkm_=cM5gt`}-7tJ^bCnR7tn zBe3b$z#jhsLv05PN&NHU+npZ4f?Y<fTmv;LxJ0w1oHl!Nb^<(FgZXuRy)xXYp~*LD z3L*PG^Lc*574Eh7oi!F-PcV5j<eaGzxJ8`ao#8ImP6c^lrB7Shz+u!o#+lun*$NN4 ze+u>1tu^^*#UCxf;bJ&NSo@TX5>5e+pT?chViS8Qddj*v2(zyvvS5|tWhS65&b1s# ztq%uj(ECDcNKU@WQlPgiVOeS{T^}+v^G!^rLMD_i<okVaaHNxvN%DKEC_OJjtJ`fy zKFE0k6@0_k?{_0d-|8&uGq1;2=W4>2e;x1$aOEia;LV&Z930_1Thek8ad0!1k*%M3 z_Q0A`8vzaWYYX7vQ=^y;COH>9zmbcNam0;YlMB3IVMi><k<II&(g=3_UBMw^HCFC+ z@)H?uigXDBXA74rM>ftHk8xY%(%}{iymL<`3XMZW+f8a2?SETgT0;aAgL=a#3!1KX z^cLebMqp!W-R{zh?QA#i^JT<kvfv0(8ry`yg<(qzh1_zt_{3BP*1~C5OmS7q)YTXx zPX4FbYU!lv4)?BM&@_$q$I|V>ju!<g<X?nd#FT-)ZWn*qLX7tafjq@`!P?D{S5IGJ zuvsiPZc%MJ!b8(+vK$@C`6S1;#b%n~3cgfSaXr-V3RSy1Czll1NJTo9Lb$}O7Z-C- zx?3&7@!8q&vQI%h9&~esojd+AF!$PiZ2Gn^%Tq%h_1PQ80p&ToqgoX4NUYYmH|n;P ztuS}UI)h4dAc_9_X4%0uX8Ct>o5tn8#754Z(}^I`q<{*5Rey^jYGXyHJRh;{p1ikj zc9Dqls^pvK0YX|_7#zPTf!*b82Bk?#z3fQBiu4q*qMiE6{UF>)MUAS7qBGw*k4&SI zpAJ7@{r3j-*FNfpyz$Rntz!RCiAZAuN`Ei6|5Fa%9ZzPgnpH&FE1=r9f!*ynU>LRk zs^i2fv8m7Ts$<41M5$$$O!aU5TsIe9FH*b*iXYp#Gs1FO^Y+cMC@x{|f&W>e{yEir zH?4mW<^(Pg@=wM6Kq2~v;(m_kcZ=>Pupg}4KY{%zU`U3zZ->A?w~ha5Y7#LChpCq# zjLs}(gwV(qHKH0PIq==G%UE&4)cV#zQp!hu3P)N#5~J*rP#)Mrnf2FEJB=>p2`NB0 zxrbCGW-cOk>(@E=<>XoI^r+-qsU_Hbj=2C;jaT42`)4Nl?$<AeJrorX|6Ggdh?op? zoEJg?^i<Tb`^gBPlWi0Ad}h<6eb;Olmg82pe%EB`<o%Hrcp--3u;QdeNlf1~)~}X+ z2_`kvu^$BMA*_~WOoBRm2Y2qwq_--k{wDL%<F=&7bN=H4xn;G>A9Ao502gVwPUdyH zk|c8N(S#6;Vbf5B1-#i5rvvuH#*!j4W%J!DHrzLL7Y}2FiA;&kE{gOSUX)eQM58e% zxzCf&=qNe#7eNq+P!z(uHeIWDRQiY=vVq^N5#m=JF!q>=l3~T}x`dQCQkTnB7Jyp9 z9<Ke9SBS3!ukGU#4CI-@!yY10$Hu`~TkU;%B9<w_?}51x8UCnvkWast$>!$S*si%Y z#K@3yE)2HKtuQf<(p%-MP;{u#ln=wpQ1YA#gF=REU4Ysw)eE4nJR61Dj5RfBZ~dM* zSMp96hovw!v!`3u@q#5n+3D*c)52Q{z19NhVU|dnO=-{cW*eIX_2Q97jaK23vWBG7 z@m-4+V3|8nS|8n5(7fuqxU~m1hiT=)sP4+=FIXUT1|kn%EzpMCxRuaG>(_h94n9E@ z^-0~gvKr&93iu$m=JxNvH`Z>5eVq0gWqPV|6Dd^9zrpc%XLqFfRKYs#OhX;3>Jm;b zh)!B^-RF<)Tr}SWQrPsyLGNqxRRqOIG#PiTgwEoM7^!y@5qBOR-Cf%U)jHUm7=x1n zabX0NS!nxd?gq)JryA$B@Ibw^qlg)E=DN@e$6aC^s8+O<N*T+sZtE3VJS6CHR;Hfb zj6!OYV>_{x@AiR~<_W*A<j@8rZy})!cUSg4dNp5pXk>_j#gJdl-^e|Hn)_#5WHWmA z*>1210pl<x|1SJEm1hL6rCToRBa2%(;ktp6={I?DPoB?_h#?1Y%pMU_Pnn5X#zFHB zkD8>k_~Sa)Oa|ASTX<>*J$=fQh1r%Sk4yV+%E8c2*ol{zZAGEORz}_=pJYbILT?~9 zx@Ww_ZY!Phs9}DZ;|(-K7u0?|<x*!^Q((Pp0U@L0j5%*kym>tedsc3u1!ajplM14D z?)FvVYF1EAD}VwEmh!>y)l!<B_<wT8$jHNU{OnXe*r~#qv;d5N2DZJiLI6N8ou4Vd g!6FL)+!7BD4?iLFVyT<d=5|Q;_y0ElgRR})0MlW=WB>pF
literal 0 HcmV?d00001
diff --git a/test/py/tests/test_ut.py b/test/py/tests/test_ut.py index 39aa1035e34..f647e2f0af2 100644 --- a/test/py/tests/test_ut.py +++ b/test/py/tests/test_ut.py @@ -28,21 +28,22 @@ def mkdir_cond(dirname): if not os.path.exists(dirname): os.mkdir(dirname)
-def setup_image(cons, mmc_dev, part_type, second_part=False): +def setup_image(cons, devnum, part_type, second_part=False, basename='mmc'): """Create a 20MB disk image with a single partition
Args: cons (ConsoleBase): Console to use - mmc_dev (int): MMC device number to use, e.g. 1 + devnum (int): Device number to use, e.g. 1 part_type (int): Partition type, e.g. 0xc for FAT32 second_part (bool): True to contain a small second partition + basename (str): Base name to use in the filename, e.g. 'mmc'
Returns: tuple: str: Filename of MMC image str: Directory name of 'mnt' directory """ - fname = os.path.join(cons.config.source_dir, f'mmc{mmc_dev}.img') + fname = os.path.join(cons.config.source_dir, f'{basename}{devnum}.img') mnt = os.path.join(cons.config.persistent_data_dir, 'mnt') mkdir_cond(mnt)
@@ -78,16 +79,17 @@ def mount_image(cons, fname, mnt, fstype): u_boot_utils.run_and_log(cons, f'sudo chown {getpass.getuser()} {mnt}') return loop
-def copy_prepared_image(cons, mmc_dev, fname): +def copy_prepared_image(cons, devnum, fname, basename='mmc'): """Use a prepared image since we cannot create one
Args: cons (ConsoleBase): Console touse - mmc_dev (int): MMC device number + devnum (int): device number fname (str): Filename of MMC image + basename (str): Base name to use in the filename, e.g. 'mmc' """ infname = os.path.join(cons.config.source_dir, - f'test/py/tests/bootstd/mmc{mmc_dev}.img.xz') + f'test/py/tests/bootstd/{basename}{devnum}.img.xz') u_boot_utils.run_and_log(cons, ['sh', '-c', f'xz -dc {infname} >{fname}'])
def setup_bootmenu_image(cons): @@ -549,6 +551,43 @@ def test_ut_dm_init(u_boot_console): with open(fn, 'wb') as fh: fh.write(data)
+ +def setup_efi_image(cons): + """Create a 20MB disk image with an EFI app on it""" + devnum = 1 + basename = 'flash' + fname, mnt = setup_image(cons, devnum, 0xc, second_part=True, + basename=basename) + + loop = None + mounted = False + complete = False + try: + loop = mount_image(cons, fname, mnt, 'ext4') + mounted = True + efi_dir = os.path.join(mnt, 'EFI') + mkdir_cond(efi_dir) + bootdir = os.path.join(efi_dir, 'BOOT') + mkdir_cond(bootdir) + efi_src = os.path.join(cons.config.build_dir, + f'lib/efi_loader/testapp.efi') + efi_dst = os.path.join(bootdir, 'BOOTSBOX.EFI') + with open(efi_src, 'rb') as inf: + with open(efi_dst, 'wb') as outf: + outf.write(inf.read()) + except ValueError as exc: + print(f'Falled to create image, failing back to prepared copy: {exc}') + + finally: + if mounted: + u_boot_utils.run_and_log(cons, 'sudo umount --lazy %s' % mnt) + if loop: + u_boot_utils.run_and_log(cons, 'sudo losetup -d %s' % loop) + + if not complete: + copy_prepared_image(cons, devnum, fname, basename) + + @pytest.mark.buildconfigspec('cmd_bootflow') @pytest.mark.buildconfigspec('sandbox') def test_ut_dm_init_bootstd(u_boot_console): @@ -559,6 +598,7 @@ def test_ut_dm_init_bootstd(u_boot_console): setup_cedit_file(u_boot_console) setup_cros_image(u_boot_console) setup_android_image(u_boot_console) + setup_efi_image(u_boot_console)
# Restart so that the new mmc1.img is picked up u_boot_console.restart_uboot()
Add a simple test of booting with the EFI bootmeth, which runs the app and checks that it can call 'exit boot-services' (to check that all the device-removal code doesn't break anything) and then exit back to U-Boot.
This uses a disk image containing the testapp, ready for execution by sandbox when needed.
Signed-off-by: Simon Glass sjg@chromium.org
---
Changes in v6: - Deal with sandbox CONFIG_LOGF_FUNC - Rebase on -next - Drop patches previously applied - Drop mention of helloworld since it is no-longer used by this test
Changes in v4: - Add efi_loader tag to some patches - Split out non-EFI patches into a different series
Changes in v2: - Add many new patches to resolve all the outstanding test issues
test/boot/bootflow.c | 63 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+)
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index e05103188b4..da2ee1ab345 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -13,6 +13,7 @@ #include <cli.h> #include <dm.h> #include <efi_default_filename.h> +#include <efi_loader.h> #include <expo.h> #ifdef CONFIG_SANDBOX #include <asm/test.h> @@ -31,6 +32,9 @@ extern U_BOOT_DRIVER(bootmeth_android); extern U_BOOT_DRIVER(bootmeth_cros); extern U_BOOT_DRIVER(bootmeth_2script);
+/* Use this as the vendor for EFI to tell the app to exit boot services */ +static u16 __efi_runtime_data test_vendor[] = u"U-Boot testing"; + static int inject_response(struct unit_test_state *uts) { /* @@ -1205,3 +1209,62 @@ static int bootflow_android(struct unit_test_state *uts) return 0; } BOOTSTD_TEST(bootflow_android, UTF_CONSOLE); + +/* Test EFI bootmeth */ +static int bootflow_efi(struct unit_test_state *uts) +{ + /* disable ethernet since the hunter will run dhcp */ + test_set_eth_enable(false); + + /* make USB scan without delays */ + test_set_skip_delays(true); + + bootstd_reset_usb(); + + /* Avoid outputting ANSI characters which mess with our asserts */ + efi_console_set_ansi(false); + + ut_assertok(bootstd_test_drop_bootdev_order(uts)); + ut_assertok(run_command("bootflow scan", 0)); + ut_assert_skip_to_line( + "Bus usb@1: scanning bus usb@1 for devices... 5 USB Device(s) found"); + + ut_assertok(run_command("bootflow list", 0)); + + ut_assert_nextlinen("Showing all"); + ut_assert_nextlinen("Seq"); + ut_assert_nextlinen("---"); + ut_assert_nextlinen(" 0 extlinux"); + ut_assert_nextlinen( + " 1 efi ready usb_mass_ 1 usb_mass_storage.lun0.boo /EFI/BOOT/BOOTSBOX.EFI"); + ut_assert_nextlinen("---"); + ut_assert_skip_to_line("(2 bootflows, 2 valid)"); + ut_assert_console_end(); + + ut_assertok(run_command("bootflow select 1", 0)); + ut_assert_console_end(); + + systab.fw_vendor = test_vendor; + + ut_asserteq(1, run_command("bootflow boot", 0)); + ut_assert_nextline( + "** Booting bootflow 'usb_mass_storage.lun0.bootdev.part_1' with efi"); + if (IS_ENABLED(CONFIG_LOGF_FUNC)) + ut_assert_skip_to_line(" efi_run_image() Booting /\EFI\BOOT\BOOTSBOX.EFI"); + else + ut_assert_skip_to_line("Booting /\EFI\BOOT\BOOTSBOX.EFI"); + + /* TODO: Why the \r ? */ + ut_assert_nextline("U-Boot test app for EFI_LOADER\r"); + ut_assert_nextline("Exiting boot sevices"); + if (IS_ENABLED(CONFIG_LOGF_FUNC)) + ut_assert_nextline(" do_bootefi_exec() ## Application failed, r = 5"); + else + ut_assert_nextline("## Application failed, r = 5"); + ut_assert_nextline("Boot failed (err=-22)"); + + ut_assert_console_end(); + + return 0; +} +BOOTSTD_TEST(bootflow_efi, UTF_CONSOLE);
On Fri, Sep 27, 2024 at 12:02:23AM +0200, Simon Glass wrote:
Add a simple test of booting with the EFI bootmeth, which runs the app and checks that it can call 'exit boot-services' (to check that all the device-removal code doesn't break anything) and then exit back to U-Boot.
This uses a disk image containing the testapp, ready for execution by sandbox when needed.
Signed-off-by: Simon Glass sjg@chromium.org
So lets go to the problem point. What asserts here fail due to ANSI escape codes being in the output and needing to be filtered out?
[snip]
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index e05103188b4..da2ee1ab345 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -13,6 +13,7 @@ #include <cli.h> #include <dm.h> #include <efi_default_filename.h> +#include <efi_loader.h> #include <expo.h> #ifdef CONFIG_SANDBOX #include <asm/test.h> @@ -31,6 +32,9 @@ extern U_BOOT_DRIVER(bootmeth_android); extern U_BOOT_DRIVER(bootmeth_cros); extern U_BOOT_DRIVER(bootmeth_2script);
+/* Use this as the vendor for EFI to tell the app to exit boot services */ +static u16 __efi_runtime_data test_vendor[] = u"U-Boot testing";
static int inject_response(struct unit_test_state *uts) { /* @@ -1205,3 +1209,62 @@ static int bootflow_android(struct unit_test_state *uts) return 0; } BOOTSTD_TEST(bootflow_android, UTF_CONSOLE);
+/* Test EFI bootmeth */ +static int bootflow_efi(struct unit_test_state *uts) +{
- /* disable ethernet since the hunter will run dhcp */
- test_set_eth_enable(false);
- /* make USB scan without delays */
- test_set_skip_delays(true);
- bootstd_reset_usb();
- /* Avoid outputting ANSI characters which mess with our asserts */
- efi_console_set_ansi(false);
- ut_assertok(bootstd_test_drop_bootdev_order(uts));
- ut_assertok(run_command("bootflow scan", 0));
- ut_assert_skip_to_line(
"Bus usb@1: scanning bus usb@1 for devices... 5 USB Device(s) found");- ut_assertok(run_command("bootflow list", 0));
- ut_assert_nextlinen("Showing all");
- ut_assert_nextlinen("Seq");
- ut_assert_nextlinen("---");
- ut_assert_nextlinen(" 0 extlinux");
- ut_assert_nextlinen(
" 1 efi ready usb_mass_ 1 usb_mass_storage.lun0.boo /EFI/BOOT/BOOTSBOX.EFI");- ut_assert_nextlinen("---");
- ut_assert_skip_to_line("(2 bootflows, 2 valid)");
- ut_assert_console_end();
- ut_assertok(run_command("bootflow select 1", 0));
- ut_assert_console_end();
- systab.fw_vendor = test_vendor;
- ut_asserteq(1, run_command("bootflow boot", 0));
- ut_assert_nextline(
"** Booting bootflow 'usb_mass_storage.lun0.bootdev.part_1' with efi");- if (IS_ENABLED(CONFIG_LOGF_FUNC))
ut_assert_skip_to_line(" efi_run_image() Booting /\\EFI\\BOOT\\BOOTSBOX.EFI");- else
ut_assert_skip_to_line("Booting /\\EFI\\BOOT\\BOOTSBOX.EFI");- /* TODO: Why the \r ? */
- ut_assert_nextline("U-Boot test app for EFI_LOADER\r");
- ut_assert_nextline("Exiting boot sevices");
- if (IS_ENABLED(CONFIG_LOGF_FUNC))
ut_assert_nextline(" do_bootefi_exec() ## Application failed, r = 5");- else
ut_assert_nextline("## Application failed, r = 5");- ut_assert_nextline("Boot failed (err=-22)");
- ut_assert_console_end();
- return 0;
+} +BOOTSTD_TEST(bootflow_efi, UTF_CONSOLE);
Hi Tom,
On Fri, 11 Oct 2024 at 16:32, Tom Rini trini@konsulko.com wrote:
On Fri, Sep 27, 2024 at 12:02:23AM +0200, Simon Glass wrote:
Add a simple test of booting with the EFI bootmeth, which runs the app and checks that it can call 'exit boot-services' (to check that all the device-removal code doesn't break anything) and then exit back to U-Boot.
This uses a disk image containing the testapp, ready for execution by sandbox when needed.
Signed-off-by: Simon Glass sjg@chromium.org
So lets go to the problem point. What asserts here fail due to ANSI escape codes being in the output and needing to be filtered out?
[snip]
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index e05103188b4..da2ee1ab345 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -13,6 +13,7 @@ #include <cli.h> #include <dm.h> #include <efi_default_filename.h> +#include <efi_loader.h> #include <expo.h> #ifdef CONFIG_SANDBOX #include <asm/test.h> @@ -31,6 +32,9 @@ extern U_BOOT_DRIVER(bootmeth_android); extern U_BOOT_DRIVER(bootmeth_cros); extern U_BOOT_DRIVER(bootmeth_2script);
+/* Use this as the vendor for EFI to tell the app to exit boot services */ +static u16 __efi_runtime_data test_vendor[] = u"U-Boot testing";
static int inject_response(struct unit_test_state *uts) { /* @@ -1205,3 +1209,62 @@ static int bootflow_android(struct unit_test_state *uts) return 0; } BOOTSTD_TEST(bootflow_android, UTF_CONSOLE);
+/* Test EFI bootmeth */ +static int bootflow_efi(struct unit_test_state *uts) +{
/* disable ethernet since the hunter will run dhcp */test_set_eth_enable(false);/* make USB scan without delays */test_set_skip_delays(true);bootstd_reset_usb();/* Avoid outputting ANSI characters which mess with our asserts */efi_console_set_ansi(false);ut_assertok(bootstd_test_drop_bootdev_order(uts));ut_assertok(run_command("bootflow scan", 0));ut_assert_skip_to_line("Bus usb@1: scanning bus usb@1 for devices... 5 USB Device(s) found");ut_assertok(run_command("bootflow list", 0));ut_assert_nextlinen("Showing all");ut_assert_nextlinen("Seq");ut_assert_nextlinen("---");ut_assert_nextlinen(" 0 extlinux");ut_assert_nextlinen(" 1 efi ready usb_mass_ 1 usb_mass_storage.lun0.boo /EFI/BOOT/BOOTSBOX.EFI");ut_assert_nextlinen("---");ut_assert_skip_to_line("(2 bootflows, 2 valid)");ut_assert_console_end();ut_assertok(run_command("bootflow select 1", 0));ut_assert_console_end();systab.fw_vendor = test_vendor;ut_asserteq(1, run_command("bootflow boot", 0));ut_assert_nextline("** Booting bootflow 'usb_mass_storage.lun0.bootdev.part_1' with efi");if (IS_ENABLED(CONFIG_LOGF_FUNC))ut_assert_skip_to_line(" efi_run_image() Booting /\\EFI\\BOOT\\BOOTSBOX.EFI");elseut_assert_skip_to_line("Booting /\\EFI\\BOOT\\BOOTSBOX.EFI");/* TODO: Why the \r ? */ut_assert_nextline("U-Boot test app for EFI_LOADER\r");ut_assert_nextline("Exiting boot sevices");if (IS_ENABLED(CONFIG_LOGF_FUNC))ut_assert_nextline(" do_bootefi_exec() ## Application failed, r = 5");elseut_assert_nextline("## Application failed, r = 5");ut_assert_nextline("Boot failed (err=-22)");ut_assert_console_end();return 0;+} +BOOTSTD_TEST(bootflow_efi, UTF_CONSOLE);
Thanks for taking an interest in this.
Nothing at present, as I added ut_assert_skip_to_line() to skip it all. But it really is just wrong...I get gibberish showing up in the terminal after I run tests! This is what I see with this series:
** Booting bootflow 'usb_mass_storage.lun0.bootdev.part_1' with efi 7 [r [999;999H [6n 8No EFI system partition
^^^ strange output there but I'm not sure if it will come through on email
No EFI system partition Failed to persist EFI variables No EFI system partition Failed to persist EFI variables No EFI system partition Failed to persist EFI variables Booting /\EFI\BOOT\BOOTSBOX.EFI U-Boot test app for EFI_LOADER
Exiting boot sevices ## Application failed, r = 5 Boot failed (err=-22) Failures: 0
-- Tom
I just want this applied or some other fix to stop it happening. It doesn't hurt anything and it should not be controversial.
Regards, Simon
On Sun, Oct 13, 2024 at 01:33:23PM -0600, Simon Glass wrote:
Hi Tom,
On Fri, 11 Oct 2024 at 16:32, Tom Rini trini@konsulko.com wrote:
On Fri, Sep 27, 2024 at 12:02:23AM +0200, Simon Glass wrote:
Add a simple test of booting with the EFI bootmeth, which runs the app and checks that it can call 'exit boot-services' (to check that all the device-removal code doesn't break anything) and then exit back to U-Boot.
This uses a disk image containing the testapp, ready for execution by sandbox when needed.
Signed-off-by: Simon Glass sjg@chromium.org
So lets go to the problem point. What asserts here fail due to ANSI escape codes being in the output and needing to be filtered out?
[snip]
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index e05103188b4..da2ee1ab345 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -13,6 +13,7 @@ #include <cli.h> #include <dm.h> #include <efi_default_filename.h> +#include <efi_loader.h> #include <expo.h> #ifdef CONFIG_SANDBOX #include <asm/test.h> @@ -31,6 +32,9 @@ extern U_BOOT_DRIVER(bootmeth_android); extern U_BOOT_DRIVER(bootmeth_cros); extern U_BOOT_DRIVER(bootmeth_2script);
+/* Use this as the vendor for EFI to tell the app to exit boot services */ +static u16 __efi_runtime_data test_vendor[] = u"U-Boot testing";
static int inject_response(struct unit_test_state *uts) { /* @@ -1205,3 +1209,62 @@ static int bootflow_android(struct unit_test_state *uts) return 0; } BOOTSTD_TEST(bootflow_android, UTF_CONSOLE);
+/* Test EFI bootmeth */ +static int bootflow_efi(struct unit_test_state *uts) +{
/* disable ethernet since the hunter will run dhcp */test_set_eth_enable(false);/* make USB scan without delays */test_set_skip_delays(true);bootstd_reset_usb();/* Avoid outputting ANSI characters which mess with our asserts */efi_console_set_ansi(false);ut_assertok(bootstd_test_drop_bootdev_order(uts));ut_assertok(run_command("bootflow scan", 0));ut_assert_skip_to_line("Bus usb@1: scanning bus usb@1 for devices... 5 USB Device(s) found");ut_assertok(run_command("bootflow list", 0));ut_assert_nextlinen("Showing all");ut_assert_nextlinen("Seq");ut_assert_nextlinen("---");ut_assert_nextlinen(" 0 extlinux");ut_assert_nextlinen(" 1 efi ready usb_mass_ 1 usb_mass_storage.lun0.boo /EFI/BOOT/BOOTSBOX.EFI");ut_assert_nextlinen("---");ut_assert_skip_to_line("(2 bootflows, 2 valid)");ut_assert_console_end();ut_assertok(run_command("bootflow select 1", 0));ut_assert_console_end();systab.fw_vendor = test_vendor;ut_asserteq(1, run_command("bootflow boot", 0));ut_assert_nextline("** Booting bootflow 'usb_mass_storage.lun0.bootdev.part_1' with efi");if (IS_ENABLED(CONFIG_LOGF_FUNC))ut_assert_skip_to_line(" efi_run_image() Booting /\\EFI\\BOOT\\BOOTSBOX.EFI");elseut_assert_skip_to_line("Booting /\\EFI\\BOOT\\BOOTSBOX.EFI");/* TODO: Why the \r ? */ut_assert_nextline("U-Boot test app for EFI_LOADER\r");ut_assert_nextline("Exiting boot sevices");if (IS_ENABLED(CONFIG_LOGF_FUNC))ut_assert_nextline(" do_bootefi_exec() ## Application failed, r = 5");elseut_assert_nextline("## Application failed, r = 5");ut_assert_nextline("Boot failed (err=-22)");ut_assert_console_end();return 0;+} +BOOTSTD_TEST(bootflow_efi, UTF_CONSOLE);
Thanks for taking an interest in this.
Nothing at present, as I added ut_assert_skip_to_line() to skip it all. But it really is just wrong...I get gibberish showing up in the terminal after I run tests! This is what I see with this series:
** Booting bootflow 'usb_mass_storage.lun0.bootdev.part_1' with efi 7 [r [999;999H [6n 8No EFI system partition
^^^ strange output there but I'm not sure if it will come through on email
No EFI system partition Failed to persist EFI variables No EFI system partition Failed to persist EFI variables No EFI system partition Failed to persist EFI variables Booting /\EFI\BOOT\BOOTSBOX.EFI U-Boot test app for EFI_LOADER
Exiting boot sevices ## Application failed, r = 5 Boot failed (err=-22) Failures: 0
So we're finally making progress I think to see what the problem you're trying to solve is. I think the next thing to figure out is if you use picocom or screen or whatever on console, the codes are handled correctly, or no? Specifically, picocom? Next, is this perhaps just another strange artifact of how oddly (and as you've noted before, slowly) pytest consumes the console input? It feels like maybe we're doing several layers of wrong there? Because to be clear, this is not a sandbox problem. Looking back at the log I had posted about the watchdog reset not counting banners correctly I see that same escape sequence, now that I know what I'm looking for/at.
All of that said, if it's not a pytest-consuming-the-console problem (and it's not a picocom problem, like I was just asking about since the board failure in question was via labgrid-client), maybe it's just a thing to note about making sure tests understand. And perhaps document somewhere why it matters (and I'm not saying it doesn't!) that we know what the dimensions of the console are, for EFI, and so that's why we init them when/where we do.
Am 14. Oktober 2024 05:51:22 MESZ schrieb Tom Rini trini@konsulko.com:
On Sun, Oct 13, 2024 at 01:33:23PM -0600, Simon Glass wrote:
Hi Tom,
On Fri, 11 Oct 2024 at 16:32, Tom Rini trini@konsulko.com wrote:
On Fri, Sep 27, 2024 at 12:02:23AM +0200, Simon Glass wrote:
Add a simple test of booting with the EFI bootmeth, which runs the app and checks that it can call 'exit boot-services' (to check that all the device-removal code doesn't break anything) and then exit back to U-Boot.
This uses a disk image containing the testapp, ready for execution by sandbox when needed.
Signed-off-by: Simon Glass sjg@chromium.org
So lets go to the problem point. What asserts here fail due to ANSI escape codes being in the output and needing to be filtered out?
[snip]
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index e05103188b4..da2ee1ab345 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -13,6 +13,7 @@ #include <cli.h> #include <dm.h> #include <efi_default_filename.h> +#include <efi_loader.h> #include <expo.h> #ifdef CONFIG_SANDBOX #include <asm/test.h> @@ -31,6 +32,9 @@ extern U_BOOT_DRIVER(bootmeth_android); extern U_BOOT_DRIVER(bootmeth_cros); extern U_BOOT_DRIVER(bootmeth_2script);
+/* Use this as the vendor for EFI to tell the app to exit boot services */ +static u16 __efi_runtime_data test_vendor[] = u"U-Boot testing";
static int inject_response(struct unit_test_state *uts) { /* @@ -1205,3 +1209,62 @@ static int bootflow_android(struct unit_test_state *uts) return 0; } BOOTSTD_TEST(bootflow_android, UTF_CONSOLE);
+/* Test EFI bootmeth */ +static int bootflow_efi(struct unit_test_state *uts) +{
/* disable ethernet since the hunter will run dhcp */test_set_eth_enable(false);/* make USB scan without delays */test_set_skip_delays(true);bootstd_reset_usb();/* Avoid outputting ANSI characters which mess with our asserts */efi_console_set_ansi(false);ut_assertok(bootstd_test_drop_bootdev_order(uts));ut_assertok(run_command("bootflow scan", 0));ut_assert_skip_to_line("Bus usb@1: scanning bus usb@1 for devices... 5 USB Device(s) found");ut_assertok(run_command("bootflow list", 0));ut_assert_nextlinen("Showing all");ut_assert_nextlinen("Seq");ut_assert_nextlinen("---");ut_assert_nextlinen(" 0 extlinux");ut_assert_nextlinen(" 1 efi ready usb_mass_ 1 usb_mass_storage.lun0.boo /EFI/BOOT/BOOTSBOX.EFI");ut_assert_nextlinen("---");ut_assert_skip_to_line("(2 bootflows, 2 valid)");ut_assert_console_end();ut_assertok(run_command("bootflow select 1", 0));ut_assert_console_end();systab.fw_vendor = test_vendor;ut_asserteq(1, run_command("bootflow boot", 0));ut_assert_nextline("** Booting bootflow 'usb_mass_storage.lun0.bootdev.part_1' with efi");if (IS_ENABLED(CONFIG_LOGF_FUNC))ut_assert_skip_to_line(" efi_run_image() Booting /\\EFI\\BOOT\\BOOTSBOX.EFI");elseut_assert_skip_to_line("Booting /\\EFI\\BOOT\\BOOTSBOX.EFI");/* TODO: Why the \r ? */ut_assert_nextline("U-Boot test app for EFI_LOADER\r");ut_assert_nextline("Exiting boot sevices");if (IS_ENABLED(CONFIG_LOGF_FUNC))ut_assert_nextline(" do_bootefi_exec() ## Application failed, r = 5");elseut_assert_nextline("## Application failed, r = 5");ut_assert_nextline("Boot failed (err=-22)");ut_assert_console_end();return 0;+} +BOOTSTD_TEST(bootflow_efi, UTF_CONSOLE);
Thanks for taking an interest in this.
Nothing at present, as I added ut_assert_skip_to_line() to skip it all. But it really is just wrong...I get gibberish showing up in the terminal after I run tests! This is what I see with this series:
** Booting bootflow 'usb_mass_storage.lun0.bootdev.part_1' with efi 7 [r [999;999H [6n 8No EFI system partition
^^^ strange output there but I'm not sure if it will come through on email
It seems you are not providing a video console. So the EFI sub-system is asking the serial console for its size in query_console_serial().
Set CONFIG_VIDEO=y and let vidconsole be your first output device to avoid query_console_serial() being called.
Best regards
Heinrich
No EFI system partition Failed to persist EFI variables No EFI system partition Failed to persist EFI variables No EFI system partition Failed to persist EFI variables Booting /\EFI\BOOT\BOOTSBOX.EFI U-Boot test app for EFI_LOADER
Exiting boot sevices ## Application failed, r = 5 Boot failed (err=-22) Failures: 0
So we're finally making progress I think to see what the problem you're trying to solve is. I think the next thing to figure out is if you use picocom or screen or whatever on console, the codes are handled correctly, or no? Specifically, picocom? Next, is this perhaps just another strange artifact of how oddly (and as you've noted before, slowly) pytest consumes the console input? It feels like maybe we're doing several layers of wrong there? Because to be clear, this is not a sandbox problem. Looking back at the log I had posted about the watchdog reset not counting banners correctly I see that same escape sequence, now that I know what I'm looking for/at.
All of that said, if it's not a pytest-consuming-the-console problem (and it's not a picocom problem, like I was just asking about since the board failure in question was via labgrid-client), maybe it's just a thing to note about making sure tests understand. And perhaps document somewhere why it matters (and I'm not saying it doesn't!) that we know what the dimensions of the console are, for EFI, and so that's why we init them when/where we do.
On Mon, Oct 14, 2024 at 09:00:40AM +0200, Heinrich Schuchardt wrote:
Am 14. Oktober 2024 05:51:22 MESZ schrieb Tom Rini trini@konsulko.com:
On Sun, Oct 13, 2024 at 01:33:23PM -0600, Simon Glass wrote:
Hi Tom,
On Fri, 11 Oct 2024 at 16:32, Tom Rini trini@konsulko.com wrote:
On Fri, Sep 27, 2024 at 12:02:23AM +0200, Simon Glass wrote:
Add a simple test of booting with the EFI bootmeth, which runs the app and checks that it can call 'exit boot-services' (to check that all the device-removal code doesn't break anything) and then exit back to U-Boot.
This uses a disk image containing the testapp, ready for execution by sandbox when needed.
Signed-off-by: Simon Glass sjg@chromium.org
So lets go to the problem point. What asserts here fail due to ANSI escape codes being in the output and needing to be filtered out?
[snip]
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index e05103188b4..da2ee1ab345 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -13,6 +13,7 @@ #include <cli.h> #include <dm.h> #include <efi_default_filename.h> +#include <efi_loader.h> #include <expo.h> #ifdef CONFIG_SANDBOX #include <asm/test.h> @@ -31,6 +32,9 @@ extern U_BOOT_DRIVER(bootmeth_android); extern U_BOOT_DRIVER(bootmeth_cros); extern U_BOOT_DRIVER(bootmeth_2script);
+/* Use this as the vendor for EFI to tell the app to exit boot services */ +static u16 __efi_runtime_data test_vendor[] = u"U-Boot testing";
static int inject_response(struct unit_test_state *uts) { /* @@ -1205,3 +1209,62 @@ static int bootflow_android(struct unit_test_state *uts) return 0; } BOOTSTD_TEST(bootflow_android, UTF_CONSOLE);
+/* Test EFI bootmeth */ +static int bootflow_efi(struct unit_test_state *uts) +{
/* disable ethernet since the hunter will run dhcp */test_set_eth_enable(false);/* make USB scan without delays */test_set_skip_delays(true);bootstd_reset_usb();/* Avoid outputting ANSI characters which mess with our asserts */efi_console_set_ansi(false);ut_assertok(bootstd_test_drop_bootdev_order(uts));ut_assertok(run_command("bootflow scan", 0));ut_assert_skip_to_line("Bus usb@1: scanning bus usb@1 for devices... 5 USB Device(s) found");ut_assertok(run_command("bootflow list", 0));ut_assert_nextlinen("Showing all");ut_assert_nextlinen("Seq");ut_assert_nextlinen("---");ut_assert_nextlinen(" 0 extlinux");ut_assert_nextlinen(" 1 efi ready usb_mass_ 1 usb_mass_storage.lun0.boo /EFI/BOOT/BOOTSBOX.EFI");ut_assert_nextlinen("---");ut_assert_skip_to_line("(2 bootflows, 2 valid)");ut_assert_console_end();ut_assertok(run_command("bootflow select 1", 0));ut_assert_console_end();systab.fw_vendor = test_vendor;ut_asserteq(1, run_command("bootflow boot", 0));ut_assert_nextline("** Booting bootflow 'usb_mass_storage.lun0.bootdev.part_1' with efi");if (IS_ENABLED(CONFIG_LOGF_FUNC))ut_assert_skip_to_line(" efi_run_image() Booting /\\EFI\\BOOT\\BOOTSBOX.EFI");elseut_assert_skip_to_line("Booting /\\EFI\\BOOT\\BOOTSBOX.EFI");/* TODO: Why the \r ? */ut_assert_nextline("U-Boot test app for EFI_LOADER\r");ut_assert_nextline("Exiting boot sevices");if (IS_ENABLED(CONFIG_LOGF_FUNC))ut_assert_nextline(" do_bootefi_exec() ## Application failed, r = 5");elseut_assert_nextline("## Application failed, r = 5");ut_assert_nextline("Boot failed (err=-22)");ut_assert_console_end();return 0;+} +BOOTSTD_TEST(bootflow_efi, UTF_CONSOLE);
Thanks for taking an interest in this.
Nothing at present, as I added ut_assert_skip_to_line() to skip it all. But it really is just wrong...I get gibberish showing up in the terminal after I run tests! This is what I see with this series:
** Booting bootflow 'usb_mass_storage.lun0.bootdev.part_1' with efi 7 [r [999;999H [6n 8No EFI system partition
^^^ strange output there but I'm not sure if it will come through on email
It seems you are not providing a video console. So the EFI sub-system is asking the serial console for its size in query_console_serial().
Set CONFIG_VIDEO=y and let vidconsole be your first output device to avoid query_console_serial() being called.
That's not an answer to the problem. Please note that rpi_arm64_defconfig shows this same issue, and it's about being able to run tests on hardware and parse the output reasonably. And so:
Best regards
Heinrich
No EFI system partition Failed to persist EFI variables No EFI system partition Failed to persist EFI variables No EFI system partition Failed to persist EFI variables Booting /\EFI\BOOT\BOOTSBOX.EFI U-Boot test app for EFI_LOADER
Exiting boot sevices ## Application failed, r = 5 Boot failed (err=-22) Failures: 0
So we're finally making progress I think to see what the problem you're trying to solve is. I think the next thing to figure out is if you use picocom or screen or whatever on console, the codes are handled correctly, or no? Specifically, picocom? Next, is this perhaps just another strange artifact of how oddly (and as you've noted before, slowly) pytest consumes the console input? It feels like maybe we're doing several layers of wrong there? Because to be clear, this is not a sandbox problem. Looking back at the log I had posted about the watchdog reset not counting banners correctly I see that same escape sequence, now that I know what I'm looking for/at.
All of that said, if it's not a pytest-consuming-the-console problem (and it's not a picocom problem, like I was just asking about since the board failure in question was via labgrid-client), maybe it's just a thing to note about making sure tests understand. And perhaps document somewhere why it matters (and I'm not saying it doesn't!) that we know what the dimensions of the console are, for EFI, and so that's why we init them when/where we do.
My last question here was to you or Ilias, thanks.
Hi Tom,
On Sun, 13 Oct 2024 at 21:51, Tom Rini trini@konsulko.com wrote:
On Sun, Oct 13, 2024 at 01:33:23PM -0600, Simon Glass wrote:
Hi Tom,
On Fri, 11 Oct 2024 at 16:32, Tom Rini trini@konsulko.com wrote:
On Fri, Sep 27, 2024 at 12:02:23AM +0200, Simon Glass wrote:
Add a simple test of booting with the EFI bootmeth, which runs the app and checks that it can call 'exit boot-services' (to check that all the device-removal code doesn't break anything) and then exit back to U-Boot.
This uses a disk image containing the testapp, ready for execution by sandbox when needed.
Signed-off-by: Simon Glass sjg@chromium.org
So lets go to the problem point. What asserts here fail due to ANSI escape codes being in the output and needing to be filtered out?
[snip]
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index e05103188b4..da2ee1ab345 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -13,6 +13,7 @@ #include <cli.h> #include <dm.h> #include <efi_default_filename.h> +#include <efi_loader.h> #include <expo.h> #ifdef CONFIG_SANDBOX #include <asm/test.h> @@ -31,6 +32,9 @@ extern U_BOOT_DRIVER(bootmeth_android); extern U_BOOT_DRIVER(bootmeth_cros); extern U_BOOT_DRIVER(bootmeth_2script);
+/* Use this as the vendor for EFI to tell the app to exit boot services */ +static u16 __efi_runtime_data test_vendor[] = u"U-Boot testing";
static int inject_response(struct unit_test_state *uts) { /* @@ -1205,3 +1209,62 @@ static int bootflow_android(struct unit_test_state *uts) return 0; } BOOTSTD_TEST(bootflow_android, UTF_CONSOLE);
+/* Test EFI bootmeth */ +static int bootflow_efi(struct unit_test_state *uts) +{
/* disable ethernet since the hunter will run dhcp */test_set_eth_enable(false);/* make USB scan without delays */test_set_skip_delays(true);bootstd_reset_usb();/* Avoid outputting ANSI characters which mess with our asserts */efi_console_set_ansi(false);ut_assertok(bootstd_test_drop_bootdev_order(uts));ut_assertok(run_command("bootflow scan", 0));ut_assert_skip_to_line("Bus usb@1: scanning bus usb@1 for devices... 5 USB Device(s) found");ut_assertok(run_command("bootflow list", 0));ut_assert_nextlinen("Showing all");ut_assert_nextlinen("Seq");ut_assert_nextlinen("---");ut_assert_nextlinen(" 0 extlinux");ut_assert_nextlinen(" 1 efi ready usb_mass_ 1 usb_mass_storage.lun0.boo /EFI/BOOT/BOOTSBOX.EFI");ut_assert_nextlinen("---");ut_assert_skip_to_line("(2 bootflows, 2 valid)");ut_assert_console_end();ut_assertok(run_command("bootflow select 1", 0));ut_assert_console_end();systab.fw_vendor = test_vendor;ut_asserteq(1, run_command("bootflow boot", 0));ut_assert_nextline("** Booting bootflow 'usb_mass_storage.lun0.bootdev.part_1' with efi");if (IS_ENABLED(CONFIG_LOGF_FUNC))ut_assert_skip_to_line(" efi_run_image() Booting /\\EFI\\BOOT\\BOOTSBOX.EFI");elseut_assert_skip_to_line("Booting /\\EFI\\BOOT\\BOOTSBOX.EFI");/* TODO: Why the \r ? */ut_assert_nextline("U-Boot test app for EFI_LOADER\r");ut_assert_nextline("Exiting boot sevices");if (IS_ENABLED(CONFIG_LOGF_FUNC))ut_assert_nextline(" do_bootefi_exec() ## Application failed, r = 5");elseut_assert_nextline("## Application failed, r = 5");ut_assert_nextline("Boot failed (err=-22)");ut_assert_console_end();return 0;+} +BOOTSTD_TEST(bootflow_efi, UTF_CONSOLE);
Thanks for taking an interest in this.
Nothing at present, as I added ut_assert_skip_to_line() to skip it all. But it really is just wrong...I get gibberish showing up in the terminal after I run tests! This is what I see with this series:
** Booting bootflow 'usb_mass_storage.lun0.bootdev.part_1' with efi 7 [r [999;999H [6n 8No EFI system partition
^^^ strange output there but I'm not sure if it will come through on email
No EFI system partition Failed to persist EFI variables No EFI system partition Failed to persist EFI variables No EFI system partition Failed to persist EFI variables Booting /\EFI\BOOT\BOOTSBOX.EFI U-Boot test app for EFI_LOADER
Exiting boot sevices ## Application failed, r = 5 Boot failed (err=-22) Failures: 0
So we're finally making progress I think to see what the problem you're trying to solve is. I think the next thing to figure out is if you use picocom or screen or whatever on console, the codes are handled correctly, or no? Specifically, picocom?
This is my running tests on my normal machine in a terminal. So it is just a sandbox connection, with no picocom, etc. See the Spawn class in test/py
Next, is this perhaps just another strange artifact of how oddly (and as you've noted before, slowly) pytest consumes the console input? It feels like maybe we're doing several layers of wrong there?
Well to be frank I would have just applied my ANSI patch some months ago and stopped worrying about it. We just shouldn't be sending ANSI characters unless we know there is a terminal there. See Color() in tools/u_boot_pylib/terminal.py
But even if we did want to blindly send characters, we should have a way to turn it off.
Because to be clear, this is not a sandbox problem. Looking back at the log I had posted about the watchdog reset not counting banners correctly I see that same escape sequence, now that I know what I'm looking for/at.
I see it all over CI as well. We also have [1].
All of that said, if it's not a pytest-consuming-the-console problem (and it's not a picocom problem, like I was just asking about since the board failure in question was via labgrid-client), maybe it's just a thing to note about making sure tests understand. And perhaps document somewhere why it matters (and I'm not saying it doesn't!) that we know what the dimensions of the console are, for EFI, and so that's why we init them when/where we do.
Or perhaps just have a way to turn it off? I first sent this patch last November. It is just wrong to generate output like this which we don't want. There isn't even a test for it, so just add a way to disable it, and be done!
Regards, SImon
[1] https://patchwork.ozlabs.org/project/uboot/patch/20240329160922.127095-1-hei...
On Mon, Oct 14, 2024 at 01:13:41PM -0600, Simon Glass wrote:
[snip]
Or perhaps just have a way to turn it off? I first sent this patch last November. It is just wrong to generate output like this which we don't want. There isn't even a test for it, so just add a way to disable it, and be done!
I don't know that it's unwanted. As I'm trying to get Heinrich to explain, _why_ does efi_setup_console_size need to exist, and do what it does? This isn't the case of color-coding the output of tests as they happen, there must be a reason we care about knowing the console size. At that point we can figure out if the right answer is: - Don't generate that check on serial ports, it's somewhere between misleading to wrong. - text-based tests just need to expect and skip it because there's a good reason to need to know the console size and not just assume 80x24. - Something else we won't know until it's clearly explained why we do this.
Date: Mon, 14 Oct 2024 15:11:50 -0600 From: Tom Rini trini@konsulko.com
On Mon, Oct 14, 2024 at 01:13:41PM -0600, Simon Glass wrote:
[snip]
Or perhaps just have a way to turn it off? I first sent this patch last November. It is just wrong to generate output like this which we don't want. There isn't even a test for it, so just add a way to disable it, and be done!
I don't know that it's unwanted. As I'm trying to get Heinrich to explain, _why_ does efi_setup_console_size need to exist, and do what it does? This isn't the case of color-coding the output of tests as they happen, there must be a reason we care about knowing the console size. At that point we can figure out if the right answer is:
- Don't generate that check on serial ports, it's somewhere between misleading to wrong.
- text-based tests just need to expect and skip it because there's a good reason to need to know the console size and not just assume 80x24.
- Something else we won't know until it's clearly explained why we do this.
Sadly this is a misfeature of UEFI. The EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL is required if console devices are supported by the UEFI implementation, and is built around the concept of "text mode" with a fixed number of character rows and columns. That pretty much assumes there is some sort of terminal emulator on the other end. And this is pretty much incompatible with anything that just want to log serial output.
I think it is possible to do better though. Instead of calling efi_setup_console_size() in efi_init_obj_list(), we could postpone this until the application makes a call that requires us to know the size. This would mean that a simple EFI application that just uses EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.OutputString() (such as the Hello world EFI application or the OpenBSD EFI bootloader) wouldn't have to do the size query.
On 15.10.24 12:19, Mark Kettenis wrote:
Date: Mon, 14 Oct 2024 15:11:50 -0600 From: Tom Rini trini@konsulko.com
On Mon, Oct 14, 2024 at 01:13:41PM -0600, Simon Glass wrote:
[snip]
Or perhaps just have a way to turn it off? I first sent this patch last November. It is just wrong to generate output like this which we don't want. There isn't even a test for it, so just add a way to disable it, and be done!
I don't know that it's unwanted. As I'm trying to get Heinrich to explain, _why_ does efi_setup_console_size need to exist, and do what it does? This isn't the case of color-coding the output of tests as they happen, there must be a reason we care about knowing the console size. At that point we can figure out if the right answer is:
- Don't generate that check on serial ports, it's somewhere between misleading to wrong.
- text-based tests just need to expect and skip it because there's a good reason to need to know the console size and not just assume 80x24.
- Something else we won't know until it's clearly explained why we do this.
Sadly this is a misfeature of UEFI. The EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL is required if console devices are supported by the UEFI implementation, and is built around the concept of "text mode" with a fixed number of character rows and columns. That pretty much assumes there is some sort of terminal emulator on the other end. And this is pretty much incompatible with anything that just want to log serial output.
I think it is possible to do better though. Instead of calling efi_setup_console_size() in efi_init_obj_list(), we could postpone this until the application makes a call that requires us to know the size. This would mean that a simple EFI application that just uses EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.OutputString() (such as the Hello world EFI application or the OpenBSD EFI bootloader) wouldn't have to do the size query.
The implementation of EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.OutputString() needs to know the output size to update the cursor position in
EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.MODE.CursorColumn EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.MODE.CursorRow
The EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL is passed to the entry point of any EFI application via the system table.
We could delay the invocation of efi_setup_console_size() to the launch of the first EFI application (efi_start_image()).
Best regards
Heinrich
Hi Heinrich,
On Tue, 15 Oct 2024 at 05:36, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 15.10.24 12:19, Mark Kettenis wrote:
Date: Mon, 14 Oct 2024 15:11:50 -0600 From: Tom Rini trini@konsulko.com
On Mon, Oct 14, 2024 at 01:13:41PM -0600, Simon Glass wrote:
[snip]
Or perhaps just have a way to turn it off? I first sent this patch last November. It is just wrong to generate output like this which we don't want. There isn't even a test for it, so just add a way to disable it, and be done!
I don't know that it's unwanted. As I'm trying to get Heinrich to explain, _why_ does efi_setup_console_size need to exist, and do what it does? This isn't the case of color-coding the output of tests as they happen, there must be a reason we care about knowing the console size. At that point we can figure out if the right answer is:
- Don't generate that check on serial ports, it's somewhere between misleading to wrong.
- text-based tests just need to expect and skip it because there's a good reason to need to know the console size and not just assume 80x24.
- Something else we won't know until it's clearly explained why we do this.
Sadly this is a misfeature of UEFI. The EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL is required if console devices are supported by the UEFI implementation, and is built around the concept of "text mode" with a fixed number of character rows and columns. That pretty much assumes there is some sort of terminal emulator on the other end. And this is pretty much incompatible with anything that just want to log serial output.
I think it is possible to do better though. Instead of calling efi_setup_console_size() in efi_init_obj_list(), we could postpone this until the application makes a call that requires us to know the size. This would mean that a simple EFI application that just uses EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.OutputString() (such as the Hello world EFI application or the OpenBSD EFI bootloader) wouldn't have to do the size query.
The implementation of EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.OutputString() needs to know the output size to update the cursor position in
EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.MODE.CursorColumn EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.MODE.CursorRow
The EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL is passed to the entry point of any EFI application via the system table.
We could delay the invocation of efi_setup_console_size() to the launch of the first EFI application (efi_start_image()).
That's a start, but I think the right answer here is to use my flag and in efi_setup_console_size() don't call query_console_serial() if the flag is set. Then the default (80x25) will be returned, which is fine for the test which is the subject of this series.
Regards, Simon
On Tue, Oct 15, 2024 at 07:25:55AM -0600, Simon Glass wrote:
Hi Heinrich,
On Tue, 15 Oct 2024 at 05:36, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 15.10.24 12:19, Mark Kettenis wrote:
Date: Mon, 14 Oct 2024 15:11:50 -0600 From: Tom Rini trini@konsulko.com
On Mon, Oct 14, 2024 at 01:13:41PM -0600, Simon Glass wrote:
[snip]
Or perhaps just have a way to turn it off? I first sent this patch last November. It is just wrong to generate output like this which we don't want. There isn't even a test for it, so just add a way to disable it, and be done!
I don't know that it's unwanted. As I'm trying to get Heinrich to explain, _why_ does efi_setup_console_size need to exist, and do what it does? This isn't the case of color-coding the output of tests as they happen, there must be a reason we care about knowing the console size. At that point we can figure out if the right answer is:
- Don't generate that check on serial ports, it's somewhere between misleading to wrong.
- text-based tests just need to expect and skip it because there's a good reason to need to know the console size and not just assume 80x24.
- Something else we won't know until it's clearly explained why we do this.
Sadly this is a misfeature of UEFI. The EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL is required if console devices are supported by the UEFI implementation, and is built around the concept of "text mode" with a fixed number of character rows and columns. That pretty much assumes there is some sort of terminal emulator on the other end. And this is pretty much incompatible with anything that just want to log serial output.
I think it is possible to do better though. Instead of calling efi_setup_console_size() in efi_init_obj_list(), we could postpone this until the application makes a call that requires us to know the size. This would mean that a simple EFI application that just uses EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.OutputString() (such as the Hello world EFI application or the OpenBSD EFI bootloader) wouldn't have to do the size query.
The implementation of EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.OutputString() needs to know the output size to update the cursor position in
EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.MODE.CursorColumn EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.MODE.CursorRow
The EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL is passed to the entry point of any EFI application via the system table.
We could delay the invocation of efi_setup_console_size() to the launch of the first EFI application (efi_start_image()).
This discussion I believe should continue as it does sound like we can be a bit better about when we do this required series of events.
That's a start, but I think the right answer here is to use my flag and in efi_setup_console_size() don't call query_console_serial() if the flag is set. Then the default (80x25) will be returned, which is fine for the test which is the subject of this series.
No, your series, like other tests that already exist and have this escape code printed, should just swallow the escape sequence if it shows up because, sadly, there is reason for it. I wonder if we should have some test _for_ this sequence, or otherwise make sure we see it when we should see it?
participants (5)
-
Heinrich Schuchardt -
Ilias Apalodimas -
Mark Kettenis -
Simon Glass -
Tom Rini