[PATCH v6 08/12] efi_loader: Disable ANSI output for tests

[PATCH v6 08/12] efi_loader: Disable ANSI output for tests

[Simon Glass]

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;
 
-- 
2.43.0

[PATCH v6 09/12] efi_loader: Add a test app

[Simon Glass]

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;
+}
-- 
2.43.0

Re: [PATCH v6 09/12] efi_loader: Add a test app

[Ilias Apalodimas]

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 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);

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
>

Re: [PATCH v6 09/12] efi_loader: Add a test app

[Simon Glass]

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 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);
>
> 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

Re: [PATCH v6 09/12] efi_loader: Add a test app

[Heinrich Schuchardt]

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 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);
>>
>> 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

Re: [PATCH v6 09/12] efi_loader: Add a test app

[Simon Glass]

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 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);
> >>
> >> 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

[PATCH v6 10/12] sandbox: virtio: Disable the sandbox virtio blk device

[Simon Glass]

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
-- 
2.43.0

Re: [PATCH v6 10/12] sandbox: virtio: Disable the sandbox virtio blk device

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 421 bytes --]

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>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH v6 11/12] test: efi: boot: Set up an image suitable for EFI testing

[Simon Glass]

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()
-- 
2.43.0

[PATCH v6 12/12] test: efi: boot: Add a test for the efi bootmeth

[Simon Glass]

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);
-- 
2.43.0

Re: [PATCH v6 12/12] test: efi: boot: Add a test for the efi bootmeth

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3573 bytes --]

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);

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v6 12/12] test: efi: boot: Add a test for the efi bootmeth

[Simon Glass]

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");
> > +     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);

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

Re: [PATCH v6 12/12] test: efi: boot: Add a test for the efi bootmeth

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 6305 bytes --]

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");
> > > +     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);
> 
> 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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v6 12/12] test: efi: boot: Add a test for the efi bootmeth

[Heinrich Schuchardt]

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");
>> > > +     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);
>> 
>> 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.
>

Re: [PATCH v6 12/12] test: efi: boot: Add a test for the efi bootmeth

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 7464 bytes --]

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");
> >> > > +     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);
> >> 
> >> 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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v6 12/12] test: efi: boot: Add a test for the efi bootmeth

[Simon Glass]

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");
> > > > +     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);
> >
> > 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-heinrich.schuchardt@canonical.com/

Re: [PATCH v6 12/12] test: efi: boot: Add a test for the efi bootmeth

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 987 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v6 12/12] test: efi: boot: Add a test for the efi bootmeth

[Mark Kettenis]

> 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.

Re: [PATCH v6 12/12] test: efi: boot: Add a test for the efi bootmeth

[Heinrich Schuchardt]

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

Re: [PATCH v6 12/12] test: efi: boot: Add a test for the efi bootmeth

[Simon Glass]

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

Re: [PATCH v6 12/12] test: efi: boot: Add a test for the efi bootmeth

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3634 bytes --]

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?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]