Hi Heinrich,

On Mon, 28 Oct 2024 at 07:07, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 10/22/24 14:00, Simon Glass wrote:

...

This test checks console output so should have the UTF_CONSOLE flag. Add it.

Signed-off-by: Simon Glass sjg@chromium.org

(no changes since v1)

test/boot/bootflow.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index 154dea70a59..cc894e804a4 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -370,7 +370,7 @@ static int bootflow_iter(struct unit_test_state

*uts)

...

  return 0;

} -BOOTSTD_TEST(bootflow_iter, UTF_DM | UTF_SCAN_FDT); +BOOTSTD_TEST(bootflow_iter, UTF_DM | UTF_SCAN_FDT | UTF_CONSOLE);

The test invokes ut_assert_console_end(). But where does it check console output?

That is it. It is making sure that there is no console output. But the flag is needed in order to do any console checking.

Regards, Simon

On 10/22/24 14:00, Simon Glass wrote:

Add a simple app to use for testing. This is intended to do whatever it needs to for testing purposes. For now it just prints a message and exits boot services.

There was a considerable amount of discussion about whether it is OK to call exit-boot-services and then return to U-Boot. This is not normally done in a real application, since exit-boot-services is used to completely disconnect from U-Boot. However, since this is a test, we need to check the results of running the app, so returning is necessary. It works correctly and it provides a nice model of how to test the EFI code in a simple way.

Signed-off-by: Simon Glass sjg@chromium.org

(no changes since v7)

Changes in v7:

• Update commit message

  lib/efi_loader/Kconfig | 10 ++++++ lib/efi_loader/Makefile | 1 + lib/efi_loader/testapp.c | 68 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+) create mode 100644 lib/efi_loader/testapp.c

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 69b2c9360d8..6ced29da719 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -565,6 +565,16 @@ config BOOTEFI_HELLO_COMPILE No additional space will be required in the resulting U-Boot binary when this option is enabled.

+config BOOTEFI_TESTAPP_COMPILE

• bool "Compile an EFI test app for testing"

• default y

• help

•  This compiles an app designed for testing. It is packed into an image
  

•  by the test.py testing frame in the setup_efi_image() function.
  

•  No additional space will be required in the resulting U-Boot binary
  

•  when this option is enabled.
  

• endif

  source "lib/efi/Kconfig"

diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile index 00d18966f9e..87131ab911d 100644 --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -20,6 +20,7 @@ apps-$(CONFIG_EFI_LOAD_FILE2_INITRD) += initrddump ifeq ($(CONFIG_GENERATE_ACPI_TABLE),) apps-y += dtbdump endif +apps-$(CONFIG_BOOTEFI_TESTAPP_COMPILE) += testapp

obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o obj-$(CONFIG_EFI_BOOTMGR) += efi_bootmgr.o diff --git a/lib/efi_loader/testapp.c b/lib/efi_loader/testapp.c new file mode 100644 index 00000000000..feb444c92e9 --- /dev/null +++ b/lib/efi_loader/testapp.c @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: GPL-2.0+

This is not a valid SPDX identifier. Please, use GPL-2.0-or-later.

+/*

• • Hello world EFI application
• • Copyright 2024 Google LLC
• • Written by Simon Glass sjg@chromium.org
• • This test program is used to test the invocation of an EFI application.
• • It writes a few messages to the console and then exits boot services
• */

+#include <efi_api.h>

+static const efi_guid_t loaded_image_guid = EFI_LOADED_IMAGE_PROTOCOL_GUID;

+static struct efi_system_table *systable; +static struct efi_boot_services *boottime; +static struct efi_simple_text_output_protocol *con_out;

+/**

• • efi_main() - entry point of the EFI application.
• • @handle: handle of the loaded image
• • @systab: system table
• • Return: status code
• */

+efi_status_t EFIAPI efi_main(efi_handle_t handle,

• 	     struct efi_system_table *systab)
  

+{

• struct efi_loaded_image *loaded_image;
• efi_status_t ret;
• efi_uintn_t map_size;
• efi_uintn_t map_key;
• efi_uintn_t desc_size;
• u32 desc_version;
• systable = systab;
• boottime = systable->boottime;
• con_out = systable->con_out;
• /* Get the loaded image protocol */
• ret = boottime->open_protocol(handle, &loaded_image_guid,

• 		      (void **)&loaded_image, NULL, NULL,
  

• 		      EFI_OPEN_PROTOCOL_GET_PROTOCOL);
  

• if (ret != EFI_SUCCESS) {

• con_out->output_string
  

• 	(con_out, u"Cannot open loaded image protocol\r\n");
  

• goto out;
  

• }
• /* UEFI requires CR LF */
• con_out->output_string(con_out, u"U-Boot test app for EFI_LOADER\r\n");

+out:

• map_size = 0;
• ret = boottime->get_memory_map(&map_size, NULL, &map_key, &desc_size,

• 		       &desc_version);
  

• con_out->output_string(con_out, u"Exiting boot sevices\n");

%s/sevices/services/g

• /* exit boot services so that this part of U-Boot can be tested */
• boottime->exit_boot_services(handle, map_key);
• /* now exit for real */
• ret = boottime->exit(handle, ret, 0, NULL);

As written before boot services are not available after ExitBootServices(). Please, call the ResetSystem() service.

• /* We should never arrive here */
• return ret;

After ExitBootServices() you must not return.

You can just do an endless loop if ResetSystem() fails:

for (;;);

Best regards

Heinrich

+}

On 10/28/24 16:17, Tom Rini wrote:

On Mon, Oct 28, 2024 at 06:59:10AM +0100, Heinrich Schuchardt wrote:

...

On 10/22/24 14:00, Simon Glass wrote:

...

Add a simple app to use for testing. This is intended to do whatever it needs to for testing purposes. For now it just prints a message and exits boot services.

There was a considerable amount of discussion about whether it is OK to call exit-boot-services and then return to U-Boot. This is not normally done in a real application, since exit-boot-services is used to completely disconnect from U-Boot. However, since this is a test, we need to check the results of running the app, so returning is necessary. It works correctly and it provides a nice model of how to test the EFI code in a simple way.

Signed-off-by: Simon Glass sjg@chromium.org

(no changes since v7)

Changes in v7:

• Update commit message

  lib/efi_loader/Kconfig | 10 ++++++ lib/efi_loader/Makefile | 1 + lib/efi_loader/testapp.c | 68 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+) create mode 100644 lib/efi_loader/testapp.c

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 69b2c9360d8..6ced29da719 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -565,6 +565,16 @@ config BOOTEFI_HELLO_COMPILE No additional space will be required in the resulting U-Boot binary when this option is enabled.

+config BOOTEFI_TESTAPP_COMPILE

• bool "Compile an EFI test app for testing"

• default y

• help

•  This compiles an app designed for testing. It is packed into an image
  

•  by the test.py testing frame in the setup_efi_image() function.
  

•  No additional space will be required in the resulting U-Boot binary
  

•  when this option is enabled.
  

• endif

  source "lib/efi/Kconfig"

diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile index 00d18966f9e..87131ab911d 100644 --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -20,6 +20,7 @@ apps-$(CONFIG_EFI_LOAD_FILE2_INITRD) += initrddump ifeq ($(CONFIG_GENERATE_ACPI_TABLE),) apps-y += dtbdump endif +apps-$(CONFIG_BOOTEFI_TESTAPP_COMPILE) += testapp

obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o obj-$(CONFIG_EFI_BOOTMGR) += efi_bootmgr.o diff --git a/lib/efi_loader/testapp.c b/lib/efi_loader/testapp.c new file mode 100644 index 00000000000..feb444c92e9 --- /dev/null +++ b/lib/efi_loader/testapp.c @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: GPL-2.0+

This is not a valid SPDX identifier. Please, use GPL-2.0-or-later.

We should switch to the proper one here, but there are numerous examples of this today.

...

...

+/*

• • Hello world EFI application
• • Copyright 2024 Google LLC
• • Written by Simon Glass sjg@chromium.org
• • This test program is used to test the invocation of an EFI application.
• • It writes a few messages to the console and then exits boot services
• */

+#include <efi_api.h>

+static const efi_guid_t loaded_image_guid = EFI_LOADED_IMAGE_PROTOCOL_GUID;

+static struct efi_system_table *systable; +static struct efi_boot_services *boottime; +static struct efi_simple_text_output_protocol *con_out;

+/**

• • efi_main() - entry point of the EFI application.
• • @handle: handle of the loaded image
• • @systab: system table
• • Return: status code
• */

+efi_status_t EFIAPI efi_main(efi_handle_t handle,

• 	     struct efi_system_table *systab)
  

+{

• struct efi_loaded_image *loaded_image;
• efi_status_t ret;
• efi_uintn_t map_size;
• efi_uintn_t map_key;
• efi_uintn_t desc_size;
• u32 desc_version;
• systable = systab;
• boottime = systable->boottime;
• con_out = systable->con_out;
• /* Get the loaded image protocol */
• ret = boottime->open_protocol(handle, &loaded_image_guid,

• 		      (void **)&loaded_image, NULL, NULL,
  

• 		      EFI_OPEN_PROTOCOL_GET_PROTOCOL);
  

• if (ret != EFI_SUCCESS) {

• con_out->output_string
  

• 	(con_out, u"Cannot open loaded image protocol\r\n");
  

• goto out;
  

• }
• /* UEFI requires CR LF */
• con_out->output_string(con_out, u"U-Boot test app for EFI_LOADER\r\n");

+out:

• map_size = 0;
• ret = boottime->get_memory_map(&map_size, NULL, &map_key, &desc_size,

• 		       &desc_version);
  

• con_out->output_string(con_out, u"Exiting boot sevices\n");

%s/sevices/services/g

...

• /* exit boot services so that this part of U-Boot can be tested */
• boottime->exit_boot_services(handle, map_key);
• /* now exit for real */
• ret = boottime->exit(handle, ret, 0, NULL);

As written before boot services are not available after ExitBootServices(). Please, call the ResetSystem() service.

Right, but the point is to not do that and instead test what happens. I think Ilias had said we need to make some big loud print on console that you must reset the system for it to be usable afterwards, would that be enough?

There is no point in testing what happens as this call is not allowable.

Do we really need to zero out the bootservices table for people who do not read the spec to find out the hard way?

Best regards

Heinrich

On 10/28/24 17:52, Tom Rini wrote:

On Mon, Oct 28, 2024 at 05:47:08PM +0100, Heinrich Schuchardt wrote:

...

On 10/28/24 16:17, Tom Rini wrote:

...

On Mon, Oct 28, 2024 at 06:59:10AM +0100, Heinrich Schuchardt wrote:

...

On 10/22/24 14:00, Simon Glass wrote:

...

Add a simple app to use for testing. This is intended to do whatever it needs to for testing purposes. For now it just prints a message and exits boot services.

There was a considerable amount of discussion about whether it is OK to call exit-boot-services and then return to U-Boot. This is not normally done in a real application, since exit-boot-services is used to completely disconnect from U-Boot. However, since this is a test, we need to check the results of running the app, so returning is necessary. It works correctly and it provides a nice model of how to test the EFI code in a simple way.

Signed-off-by: Simon Glass sjg@chromium.org

(no changes since v7)

Changes in v7:

• Update commit message

  lib/efi_loader/Kconfig | 10 ++++++ lib/efi_loader/Makefile | 1 + lib/efi_loader/testapp.c | 68 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+) create mode 100644 lib/efi_loader/testapp.c

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 69b2c9360d8..6ced29da719 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -565,6 +565,16 @@ config BOOTEFI_HELLO_COMPILE No additional space will be required in the resulting U-Boot binary when this option is enabled.

+config BOOTEFI_TESTAPP_COMPILE

• bool "Compile an EFI test app for testing"

• default y

• help

•  This compiles an app designed for testing. It is packed into an image
  

•  by the test.py testing frame in the setup_efi_image() function.
  

•  No additional space will be required in the resulting U-Boot binary
  

•  when this option is enabled.
  

• endif

  source "lib/efi/Kconfig"

diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile index 00d18966f9e..87131ab911d 100644 --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -20,6 +20,7 @@ apps-$(CONFIG_EFI_LOAD_FILE2_INITRD) += initrddump ifeq ($(CONFIG_GENERATE_ACPI_TABLE),) apps-y += dtbdump endif +apps-$(CONFIG_BOOTEFI_TESTAPP_COMPILE) += testapp

obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
obj-$(CONFIG_EFI_BOOTMGR) += efi_bootmgr.o

diff --git a/lib/efi_loader/testapp.c b/lib/efi_loader/testapp.c new file mode 100644 index 00000000000..feb444c92e9 --- /dev/null +++ b/lib/efi_loader/testapp.c @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: GPL-2.0+

This is not a valid SPDX identifier. Please, use GPL-2.0-or-later.

We should switch to the proper one here, but there are numerous examples of this today.

...

...

+/*

• • Hello world EFI application
• • Copyright 2024 Google LLC
• • Written by Simon Glass sjg@chromium.org
• • This test program is used to test the invocation of an EFI application.
• • It writes a few messages to the console and then exits boot services
• */

+#include <efi_api.h>

+static const efi_guid_t loaded_image_guid = EFI_LOADED_IMAGE_PROTOCOL_GUID;

+static struct efi_system_table *systable; +static struct efi_boot_services *boottime; +static struct efi_simple_text_output_protocol *con_out;

+/**

• • efi_main() - entry point of the EFI application.
• • @handle: handle of the loaded image
• • @systab: system table
• • Return: status code
• */

+efi_status_t EFIAPI efi_main(efi_handle_t handle,

• 	     struct efi_system_table *systab)
  

+{

• struct efi_loaded_image *loaded_image;
• efi_status_t ret;
• efi_uintn_t map_size;
• efi_uintn_t map_key;
• efi_uintn_t desc_size;
• u32 desc_version;
• systable = systab;
• boottime = systable->boottime;
• con_out = systable->con_out;
• /* Get the loaded image protocol */
• ret = boottime->open_protocol(handle, &loaded_image_guid,

• 		      (void **)&loaded_image, NULL, NULL,
  

• 		      EFI_OPEN_PROTOCOL_GET_PROTOCOL);
  

• if (ret != EFI_SUCCESS) {

• con_out->output_string
  

• 	(con_out, u"Cannot open loaded image protocol\r\n");
  

• goto out;
  

• }
• /* UEFI requires CR LF */
• con_out->output_string(con_out, u"U-Boot test app for EFI_LOADER\r\n");

+out:

• map_size = 0;
• ret = boottime->get_memory_map(&map_size, NULL, &map_key, &desc_size,

• 		       &desc_version);
  

• con_out->output_string(con_out, u"Exiting boot sevices\n");

%s/sevices/services/g

...

• /* exit boot services so that this part of U-Boot can be tested */
• boottime->exit_boot_services(handle, map_key);
• /* now exit for real */
• ret = boottime->exit(handle, ret, 0, NULL);

As written before boot services are not available after ExitBootServices(). Please, call the ResetSystem() service.

Right, but the point is to not do that and instead test what happens. I think Ilias had said we need to make some big loud print on console that you must reset the system for it to be usable afterwards, would that be enough?

There is no point in testing what happens as this call is not allowable.

Do we really need to zero out the bootservices table for people who do not read the spec to find out the hard way?

Then perhaps this is a case where "test causes U-Boot to reset, check for that as the result" is what the test needs to do, as it's not an arbitrary reset?

Yes, we should check that a reset occurs after ResetSystem(). Currently we are only testing that the EFI watchdog resets the system in test_efi_selftest_watchdog_reboot().

As on all non-sandbox systems all devices are removed at ExitBootServices() this is the only way forward to write a test that works on all UEFI supporting U-Boot systems.

Best regards

Heinrich

On 10/28/24 18:00, Simon Glass wrote:

Hi Heinrich,

On Mon, 28 Oct 2024 at 07:11, Heinrich Schuchardt <xypron.glpk@gmx.de mailto:xypron.glpk@gmx.de> wrote:

...

On 10/22/24 14:00, Simon Glass wrote:

...

Add a simple app to use for testing. This is intended to do whatever it needs to for testing purposes. For now it just prints a message and exits boot services.

There was a considerable amount of discussion about whether it is OK to call exit-boot-services and then return to U-Boot. This is not normally done in a real application, since exit-boot-services is used to completely disconnect from U-Boot. However, since this is a test, we need to check the results of running the app, so returning is

necessary.

...

...

It works correctly and it provides a nice model of how to test the EFI code in a simple way.

Signed-off-by: Simon Glass <sjg@chromium.org mailto:sjg@chromium.org>

(no changes since v7)

Changes in v7:

• Update commit message

lib/efi_loader/Kconfig   | 10 ++++++    lib/efi_loader/Makefile  |  1 +    lib/efi_loader/testapp.c | 68 +++++++++++++++++++++++++++++++++++

+++++

...

...

3 files changed, 79 insertions(+)    create mode 100644 lib/efi_loader/testapp.c

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 69b2c9360d8..6ced29da719 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -565,6 +565,16 @@ config BOOTEFI_HELLO_COMPILE          No additional space will be required in the resulting U-

Boot binary

...

...

when this option is enabled.

+config BOOTEFI_TESTAPP_COMPILE +     bool "Compile an EFI test app for testing" +     default y +     help +       This compiles an app designed for testing. It is packed

into an image

...

...

+       by the test.py testing frame in the setup_efi_image() function.

+       No additional space will be required in the resulting U-

Boot binary

...

...

+       when this option is enabled.

endif

source "lib/efi/Kconfig" diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile index 00d18966f9e..87131ab911d 100644 --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -20,6 +20,7 @@ apps-$(CONFIG_EFI_LOAD_FILE2_INITRD) += initrddump    ifeq ($(CONFIG_GENERATE_ACPI_TABLE),)    apps-y += dtbdump    endif +apps-$(CONFIG_BOOTEFI_TESTAPP_COMPILE) += testapp

obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o    obj-$(CONFIG_EFI_BOOTMGR) += efi_bootmgr.o diff --git a/lib/efi_loader/testapp.c b/lib/efi_loader/testapp.c new file mode 100644 index 00000000000..feb444c92e9 --- /dev/null +++ b/lib/efi_loader/testapp.c @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: GPL-2.0+

This is not a valid SPDX identifier. Please, use GPL-2.0-or-later.

...

+/*

• • Hello world EFI application
• • Copyright 2024 Google LLC
• • Written by Simon Glass <sjg@chromium.org mailto:sjg@chromium.org>
• • This test program is used to test the invocation of an EFI

application.

...

...

• • It writes a few messages to the console and then exits boot

services

...

...

• */

+#include <efi_api.h>

+static const efi_guid_t loaded_image_guid =

EFI_LOADED_IMAGE_PROTOCOL_GUID;

...

...

+static struct efi_system_table *systable; +static struct efi_boot_services *boottime; +static struct efi_simple_text_output_protocol *con_out;

+/**

• • efi_main() - entry point of the EFI application.
• • @handle:  handle of the loaded image
• • @systab:  system table
• • Return:   status code
• */

+efi_status_t EFIAPI efi_main(efi_handle_t handle, +                          struct efi_system_table *systab) +{ +     struct efi_loaded_image *loaded_image; +     efi_status_t ret; +     efi_uintn_t map_size; +     efi_uintn_t map_key; +     efi_uintn_t desc_size; +     u32 desc_version;

+     systable = systab; +     boottime = systable->boottime; +     con_out = systable->con_out;

+     /* Get the loaded image protocol */ +     ret = boottime->open_protocol(handle, &loaded_image_guid, +                                   (void **)&loaded_image, NULL, NULL, +                                   EFI_OPEN_PROTOCOL_GET_PROTOCOL); +     if (ret != EFI_SUCCESS) { +             con_out->output_string +                     (con_out, u"Cannot open loaded image

protocol\r\n");

...

...

+             goto out; +     }

+     /* UEFI requires CR LF */ +     con_out->output_string(con_out, u"U-Boot test app for

EFI_LOADER\r\n");

...

...

+out: +     map_size = 0; +     ret = boottime->get_memory_map(&map_size, NULL, &map_key,

&desc_size,

...

...

+                                    &desc_version); +     con_out->output_string(con_out, u"Exiting boot sevices\n");

%s/sevices/services/g

...

+     /* exit boot services so that this part of U-Boot can be

tested */

...

...

+     boottime->exit_boot_services(handle, map_key);

+     /* now exit for real */ +     ret = boottime->exit(handle, ret, 0, NULL);

As written before boot services are not available after ExitBootServices(). Please, call the ResetSystem() service.

I already explained this. If I call reset then U-Boot exits and the console output cannot be checked by the C test. If I tell sandbox's reset-driver to ignore the reset request, then it also doesn't come back to the test.

...

...

+     /* We should never arrive here */ +     return ret;

After ExitBootServices() you must not return.

You can just do an endless loop if ResetSystem() fails:

for (;;);

No, because I need to check the output of the app. Please can you try to run this test so you can see what it is doing?

The Python framework lets you check the output whether you are rebooting or not.

The test seems not to build on anything but sandbox. We should be able to test the boot methods on QEMU systems.

In efi_exit_boot_services() we

* call dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL) * call bootm_disable_interrupts() * call board_quiesce_devices() * disable all EFI timers * disable EFI notification

What do you expect U-Boot to do after all of this? You cannot reasonably do any follow on test without rebooting.

EDK II does the following in ExitBootServices():

// // Clear the non-runtime values of the EFI System Table // gDxeCoreST->BootServices = NULL; gDxeCoreST->ConIn = NULL; gDxeCoreST->ConsoleInHandle = NULL; gDxeCoreST->ConOut = NULL; gDxeCoreST->ConsoleOutHandle = NULL; gDxeCoreST->StdErr = NULL; gDxeCoreST->StandardErrorHandle = NULL;

As said ExitBootServices is the point of no-return.

Please, write a compliant test that runs on all UEFI architectures. Then we can review it further.

Best regards

Heinrich

On Tue, Oct 29, 2024 at 04:46:16PM +0100, Simon Glass wrote:

Hi Heinrich,

On Mon, 28 Oct 2024 at 19:43, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 10/28/24 18:00, Simon Glass wrote:

...

Hi Heinrich,

On Mon, 28 Oct 2024 at 07:11, Heinrich Schuchardt <xypron.glpk@gmx.de mailto:xypron.glpk@gmx.de> wrote:

...

On 10/22/24 14:00, Simon Glass wrote:

...

Add a simple app to use for testing. This is intended to do whatever it needs to for testing purposes. For now it just prints a message and exits boot services.

There was a considerable amount of discussion about whether it is OK to call exit-boot-services and then return to U-Boot. This is not normally done in a real application, since exit-boot-services is used to completely disconnect from U-Boot. However, since this is a test, we need to check the results of running the app, so returning is

necessary.

...

...

It works correctly and it provides a nice model of how to test the EFI code in a simple way.

Signed-off-by: Simon Glass <sjg@chromium.org mailto:sjg@chromium.org>

(no changes since v7)

Changes in v7:

• Update commit message

  lib/efi_loader/Kconfig | 10 ++++++ lib/efi_loader/Makefile | 1 + lib/efi_loader/testapp.c | 68 +++++++++++++++++++++++++++++++++++

+++++

...

...

3 files changed, 79 insertions(+) create mode 100644 lib/efi_loader/testapp.c

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 69b2c9360d8..6ced29da719 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -565,6 +565,16 @@ config BOOTEFI_HELLO_COMPILE No additional space will be required in the resulting U-

Boot binary

...

...

    when this option is enabled.

+config BOOTEFI_TESTAPP_COMPILE

• bool "Compile an EFI test app for testing"
  

• default y
  

• help
  

•   This compiles an app designed for testing. It is packed
  

into an image

...

...

•   by the test.py testing frame in the setup_efi_image() function.
  

•   No additional space will be required in the resulting U-
  

Boot binary

...

...

•   when this option is enabled.
  

• endif

  source "lib/efi/Kconfig"

diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile index 00d18966f9e..87131ab911d 100644 --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -20,6 +20,7 @@ apps-$(CONFIG_EFI_LOAD_FILE2_INITRD) += initrddump ifeq ($(CONFIG_GENERATE_ACPI_TABLE),) apps-y += dtbdump endif +apps-$(CONFIG_BOOTEFI_TESTAPP_COMPILE) += testapp

obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o obj-$(CONFIG_EFI_BOOTMGR) += efi_bootmgr.o diff --git a/lib/efi_loader/testapp.c b/lib/efi_loader/testapp.c new file mode 100644 index 00000000000..feb444c92e9 --- /dev/null +++ b/lib/efi_loader/testapp.c @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: GPL-2.0+

This is not a valid SPDX identifier. Please, use GPL-2.0-or-later.

...

+/*

• • Hello world EFI application
• • Copyright 2024 Google LLC
• • Written by Simon Glass <sjg@chromium.org mailto:sjg@chromium.org>
• • This test program is used to test the invocation of an EFI

application.

...

...

• • It writes a few messages to the console and then exits boot

services

...

...

• */

+#include <efi_api.h>

+static const efi_guid_t loaded_image_guid =

EFI_LOADED_IMAGE_PROTOCOL_GUID;

...

...

+static struct efi_system_table *systable; +static struct efi_boot_services *boottime; +static struct efi_simple_text_output_protocol *con_out;

+/**

• • efi_main() - entry point of the EFI application.
• • @handle: handle of the loaded image
• • @systab: system table
• • Return: status code
• */

+efi_status_t EFIAPI efi_main(efi_handle_t handle,

•                      struct efi_system_table *systab)
  

+{

• struct efi_loaded_image *loaded_image;
  

• efi_status_t ret;
  

• efi_uintn_t map_size;
  

• efi_uintn_t map_key;
  

• efi_uintn_t desc_size;
  

• u32 desc_version;
  

• systable = systab;
  

• boottime = systable->boottime;
  

• con_out = systable->con_out;
  

• /* Get the loaded image protocol */
  

• ret = boottime->open_protocol(handle, &loaded_image_guid,
  

•                               (void **)&loaded_image, NULL, NULL,
  

•                               EFI_OPEN_PROTOCOL_GET_PROTOCOL);
  

• if (ret != EFI_SUCCESS) {
  

•         con_out->output_string
  

•                 (con_out, u"Cannot open loaded image
  

protocol\r\n");

...

...

•         goto out;
  

• }
  

• /* UEFI requires CR LF */
  

• con_out->output_string(con_out, u"U-Boot test app for
  

EFI_LOADER\r\n");

...

...

+out:

• map_size = 0;
  

• ret = boottime->get_memory_map(&map_size, NULL, &map_key,
  

&desc_size,

...

...

•                                &desc_version);
  

• con_out->output_string(con_out, u"Exiting boot sevices\n");
  

%s/sevices/services/g

...

• /* exit boot services so that this part of U-Boot can be
  

tested */

...

...

• boottime->exit_boot_services(handle, map_key);
  

• /* now exit for real */
  

• ret = boottime->exit(handle, ret, 0, NULL);
  

As written before boot services are not available after ExitBootServices(). Please, call the ResetSystem() service.

I already explained this. If I call reset then U-Boot exits and the console output cannot be checked by the C test. If I tell sandbox's reset-driver to ignore the reset request, then it also doesn't come back to the test.

...

...

• /* We should never arrive here */
  

• return ret;
  

After ExitBootServices() you must not return.

You can just do an endless loop if ResetSystem() fails:

for (;;);

No, because I need to check the output of the app. Please can you try to run this test so you can see what it is doing?

The Python framework lets you check the output whether you are rebooting or not.

Yes, I complained about catching reboots at the time, but it was submitted against my wishes. It is not a good design to create a problem in a test and then work around it later. It is easier to just return, rather than try to catch a reset in the Python test. For one thing, it makes using gdb much easier to have the test be self-contained.

And sometimes a test needs to restart the actual system for the test. This sounds like another case of that, and not some arbitrary bad design restart.

To phrase that another way, you're trying to introduce workarounds in the code to avoid having to make the test work like a real system. I do not think that's a good idea for a test.

...

The test seems not to build on anything but sandbox. We should be able to test the boot methods on QEMU systems.

Most tests run only on sandbox. You are welcome to extend the tests to cover more architectures. However I believe the most important thing is to test the code itself.

We also need to write things in such a way that we don't have to start over from scratch when testing on other emulation platforms. It's true that it's unlikely anyone will run the full cycle of booting various off the shelf distributions and architectures and boot media for every change. But I've got ~2 hours between assembling a pull request and looking at the build results and anything I can throw at a host and come back to in that time I'll do every time. Things that are automated but marginally longer I'll do with every tagged release.

...

In efi_exit_boot_services() we

• call dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL)
• call bootm_disable_interrupts()
• call board_quiesce_devices()
• disable all EFI timers
• disable EFI notification

What do you expect U-Boot to do after all of this? You cannot reasonably do any follow on test without rebooting.

OK, well we can always add more code later as needed. So far, this seems to pass CI without problems.

What EFI-based tests do you run on sandbox after this, without resetting the system? That would perhaps help clarify the point.

On Wed, Oct 30, 2024 at 08:41:27AM +0100, Simon Glass wrote:

Hi Tom,

On Tue, 29 Oct 2024 at 17:46, Tom Rini trini@konsulko.com wrote:

...

On Tue, Oct 29, 2024 at 04:46:16PM +0100, Simon Glass wrote:

...

Hi Heinrich,

On Mon, 28 Oct 2024 at 19:43, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 10/28/24 18:00, Simon Glass wrote:

...

Hi Heinrich,

On Mon, 28 Oct 2024 at 07:11, Heinrich Schuchardt <xypron.glpk@gmx.de mailto:xypron.glpk@gmx.de> wrote:

...

On 10/22/24 14:00, Simon Glass wrote: > Add a simple app to use for testing. This is intended to do whatever it > needs to for testing purposes. For now it just prints a message and > exits boot services. > > There was a considerable amount of discussion about whether it is OK to > call exit-boot-services and then return to U-Boot. This is not normally > done in a real application, since exit-boot-services is used to > completely disconnect from U-Boot. However, since this is a test, we > need to check the results of running the app, so returning is

necessary.

...

> It works correctly and it provides a nice model of how to test the EFI > code in a simple way. > > Signed-off-by: Simon Glass <sjg@chromium.org mailto:sjg@chromium.org> > --- > > (no changes since v7) > > Changes in v7: > - Update commit message > > lib/efi_loader/Kconfig | 10 ++++++ > lib/efi_loader/Makefile | 1 + > lib/efi_loader/testapp.c | 68 +++++++++++++++++++++++++++++++++++

+++++

...

> 3 files changed, 79 insertions(+) > create mode 100644 lib/efi_loader/testapp.c > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > index 69b2c9360d8..6ced29da719 100644 > --- a/lib/efi_loader/Kconfig > +++ b/lib/efi_loader/Kconfig > @@ -565,6 +565,16 @@ config BOOTEFI_HELLO_COMPILE > No additional space will be required in the resulting U-

Boot binary

...

> when this option is enabled. > > +config BOOTEFI_TESTAPP_COMPILE > + bool "Compile an EFI test app for testing" > + default y > + help > + This compiles an app designed for testing. It is packed

into an image

...

> + by the test.py testing frame in the setup_efi_image() function. > + > + No additional space will be required in the resulting U-

Boot binary

...

> + when this option is enabled. > + > endif > > source "lib/efi/Kconfig" > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile > index 00d18966f9e..87131ab911d 100644 > --- a/lib/efi_loader/Makefile > +++ b/lib/efi_loader/Makefile > @@ -20,6 +20,7 @@ apps-$(CONFIG_EFI_LOAD_FILE2_INITRD) += initrddump > ifeq ($(CONFIG_GENERATE_ACPI_TABLE),) > apps-y += dtbdump > endif > +apps-$(CONFIG_BOOTEFI_TESTAPP_COMPILE) += testapp > > obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o > obj-$(CONFIG_EFI_BOOTMGR) += efi_bootmgr.o > diff --git a/lib/efi_loader/testapp.c b/lib/efi_loader/testapp.c > new file mode 100644 > index 00000000000..feb444c92e9 > --- /dev/null > +++ b/lib/efi_loader/testapp.c > @@ -0,0 +1,68 @@ > +// SPDX-License-Identifier: GPL-2.0+

This is not a valid SPDX identifier. Please, use GPL-2.0-or-later.

> +/* > + * Hello world EFI application > + * > + * Copyright 2024 Google LLC > + * Written by Simon Glass <sjg@chromium.org mailto:sjg@chromium.org> > + * > + * This test program is used to test the invocation of an EFI

application.

...

> + * It writes a few messages to the console and then exits boot

services

...

> + */ > + > +#include <efi_api.h> > + > +static const efi_guid_t loaded_image_guid =

EFI_LOADED_IMAGE_PROTOCOL_GUID;

...

> + > +static struct efi_system_table *systable; > +static struct efi_boot_services *boottime; > +static struct efi_simple_text_output_protocol *con_out; > + > +/** > + * efi_main() - entry point of the EFI application. > + * > + * @handle: handle of the loaded image > + * @systab: system table > + * Return: status code > + */ > +efi_status_t EFIAPI efi_main(efi_handle_t handle, > + struct efi_system_table *systab) > +{ > + struct efi_loaded_image *loaded_image; > + efi_status_t ret; > + efi_uintn_t map_size; > + efi_uintn_t map_key; > + efi_uintn_t desc_size; > + u32 desc_version; > + > + systable = systab; > + boottime = systable->boottime; > + con_out = systable->con_out; > + > + /* Get the loaded image protocol */ > + ret = boottime->open_protocol(handle, &loaded_image_guid, > + (void **)&loaded_image, NULL, NULL, > + EFI_OPEN_PROTOCOL_GET_PROTOCOL); > + if (ret != EFI_SUCCESS) { > + con_out->output_string > + (con_out, u"Cannot open loaded image

protocol\r\n");

...

> + goto out; > + } > + > + /* UEFI requires CR LF */ > + con_out->output_string(con_out, u"U-Boot test app for

EFI_LOADER\r\n");

...

> + > +out: > + map_size = 0; > + ret = boottime->get_memory_map(&map_size, NULL, &map_key,

&desc_size,

...

> + &desc_version); > + con_out->output_string(con_out, u"Exiting boot sevices\n");

%s/sevices/services/g

> + > + /* exit boot services so that this part of U-Boot can be

tested */

...

> + boottime->exit_boot_services(handle, map_key); > + > + /* now exit for real */ > + ret = boottime->exit(handle, ret, 0, NULL);

As written before boot services are not available after ExitBootServices(). Please, call the ResetSystem() service.

I already explained this. If I call reset then U-Boot exits and the console output cannot be checked by the C test. If I tell sandbox's reset-driver to ignore the reset request, then it also doesn't come back to the test.

...

> + > + /* We should never arrive here */ > + return ret;

After ExitBootServices() you must not return.

You can just do an endless loop if ResetSystem() fails:

for (;;);

No, because I need to check the output of the app. Please can you try to run this test so you can see what it is doing?

The Python framework lets you check the output whether you are rebooting or not.

Yes, I complained about catching reboots at the time, but it was submitted against my wishes. It is not a good design to create a problem in a test and then work around it later. It is easier to just return, rather than try to catch a reset in the Python test. For one thing, it makes using gdb much easier to have the test be self-contained.

And sometimes a test needs to restart the actual system for the test. This sounds like another case of that, and not some arbitrary bad design restart.

Sorry, but I cannot tell the difference. We should not have tests which need to restart, except in extreme circumstances.

That's unfortunate then. Perhaps you'll just have to trust me that there are in fact tests where it's only valid to restart the system and not fake restart the system.

...

To phrase that another way, you're trying to introduce workarounds in the code to avoid having to make the test work like a real system. I do not think that's a good idea for a test.

Which work-around are you referring to?

Everything you're doing here.

I discussed this somewhat with Heinrich earlier. We didn't really resolve it, but I did point out that if we want to reset/reinit the EFI system within U-Boot (as we do with driver model, devicetree and some other things) we can implement that when it is needed.

That would be even more workaround.

...

...

...

The test seems not to build on anything but sandbox. We should be able to test the boot methods on QEMU systems.

Most tests run only on sandbox. You are welcome to extend the tests to cover more architectures. However I believe the most important thing is to test the code itself.

We also need to write things in such a way that we don't have to start over from scratch when testing on other emulation platforms. It's true that it's unlikely anyone will run the full cycle of booting various off the shelf distributions and architectures and boot media for every change. But I've got ~2 hours between assembling a pull request and looking at the build results and anything I can throw at a host and come back to in that time I'll do every time. Things that are automated but marginally longer I'll do with every tagged release.

I'm not sure how best to attack this, even now. With the Labgrid integration (I am still hoping will land soon) I can boot into QEMU and perhaps into grub, but booting right into a distro is going to need ssh and networking (which is fine, it's just another step).

Anyway, that is a bid sideways of your point. Sandbox tests are always going to be much easier to write than Python ones. They are faster to run, as well. We can't do things like 'assert_nextline()' with QEMU. Also I can't single step into QEMU with the debugger. Also with sandbox, it is possible to check the internal U-Boot-state, whereas with QEMU we have to print something on the console. So I believe that writing a QEMU test is an entirely different job from writing a sandbox test.

I'm not sure how this is at all relevant to the point here. If you make the test application follow the UEFI spec this is no different from the existing hello world test we have in most respects.

There is something fundamental here which can be changed. Sandbox is really just better for testing U-Boot code, so long as we design the code properly for testing. Of course if we insist on not designing the code properly for testing, that erodes the advantage. But that would be the wrong direction.

To be frank, I haven't been able to run the whole sandbox test suite locally in the last 5+ years if ever, and had to throw it in to docker, and then just got tired of fighting it and only see it in CI.

...

...

...

In efi_exit_boot_services() we

• call dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL)
• call bootm_disable_interrupts()
• call board_quiesce_devices()
• disable all EFI timers
• disable EFI notification

What do you expect U-Boot to do after all of this? You cannot reasonably do any follow on test without rebooting.

OK, well we can always add more code later as needed. So far, this seems to pass CI without problems.

What EFI-based tests do you run on sandbox after this, without resetting the system? That would perhaps help clarify the point.

It depends on the ordering of things in CI. From what I can tell most (or all?) of them run after this test.

Well, you shouldn't and I think earlier Heinrich was saying that perhaps we need to NULL out a few things more then.

But if this does cause a problem with future tests that we add, we should be able to fix it easily enough. In fact it would be nice to be able to re-init EFI.

Sounds like a lot of work-around work just to avoid testing that we are in fact exiting services and resetting like we should.

Hi Simon,

On Tue, 22 Oct 2024 at 15:00, Simon Glass sjg@chromium.org wrote:

Use a function to obtain the device EFI filename, so that we can control how sandbox behaves.

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v8:

• Add new patch to move default filename to a function

boot/Makefile | 4 +++- boot/bootmeth_efi.c | 4 ++-- include/efi_default_filename.h => boot/efi_fname.c | 11 +++++------ include/efi.h | 9 +++++++++ lib/efi_loader/efi_bootmgr.c | 10 +++++++--- test/boot/bootflow.c | 6 +++--- 6 files changed, 29 insertions(+), 15 deletions(-) rename include/efi_default_filename.h => boot/efi_fname.c (93%)

Let's not spread efi files all around the place. Can you add the function you want in lib/efi_loader/efi_helped.c which is there to add helper files?

diff --git a/boot/Makefile b/boot/Makefile index b24f806d5bf..d3f2c02068c 100644 --- a/boot/Makefile +++ b/boot/Makefile @@ -28,7 +28,7 @@ obj-$(CONFIG_$(PHASE_)BOOTSTD_PROG) += prog_boot.o

obj-$(CONFIG_$(PHASE_)BOOTMETH_EXTLINUX) += bootmeth_extlinux.o obj-$(CONFIG_$(PHASE_)BOOTMETH_EXTLINUX_PXE) += bootmeth_pxe.o -obj-$(CONFIG_$(PHASE_)BOOTMETH_EFILOADER) += bootmeth_efi.o +obj-$(CONFIG_$(PHASE_)BOOTMETH_EFILOADER) += bootmeth_efi.o efi_fname.o obj-$(CONFIG_$(PHASE_)BOOTMETH_CROS) += bootm.o bootm_os.o bootmeth_cros.o obj-$(CONFIG_$(PHASE_)BOOTMETH_QFW) += bootmeth_qfw.o obj-$(CONFIG_$(PHASE_)BOOTMETH_SANDBOX) += bootmeth_sandbox.o @@ -40,6 +40,8 @@ obj-$(CONFIG_$(PHASE_)EXPO) += bootflow_menu.o obj-$(CONFIG_$(PHASE_)BOOTSTD) += bootflow_menu.o endif

+obj-$(CONFIG_EFI_LOADER) += efi_fname.o

obj-$(CONFIG_$(PHASE_)OF_LIBFDT) += fdt_support.o obj-$(CONFIG_$(PHASE_)FDT_SIMPLEFB) += fdt_simplefb.o

diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c index 2ad6d3b4ace..371b36d550b 100644 --- a/boot/bootmeth_efi.c +++ b/boot/bootmeth_efi.c @@ -13,7 +13,7 @@ #include <bootmeth.h> #include <command.h> #include <dm.h> -#include <efi_default_filename.h> +#include <efi.h> #include <efi_loader.h> #include <fs.h> #include <malloc.h> @@ -168,7 +168,7 @@ static int distro_efi_try_bootflow_files(struct udevice *dev, }

    strcpy(fname, EFI_DIRNAME);

•   strcat(fname, BOOTEFI_NAME);
  

•   strcat(fname, efi_get_basename());
  
    if (bflow->blk)
             desc = dev_get_uclass_plat(bflow->blk);
  

diff --git a/include/efi_default_filename.h b/boot/efi_fname.c similarity index 93% rename from include/efi_default_filename.h rename to boot/efi_fname.c index 77932984b55..a6b11383bba 100644 --- a/include/efi_default_filename.h +++ b/boot/efi_fname.c @@ -8,13 +8,9 @@

• Copyright (c) 2022, Linaro Limited

*/

-#ifndef _EFI_DEFAULT_FILENAME_H -#define _EFI_DEFAULT_FILENAME_H

+#include <efi.h> #include <host_arch.h>

-#undef BOOTEFI_NAME

#ifdef CONFIG_SANDBOX

#if HOST_ARCH == HOST_ARCH_X86_64 @@ -53,4 +49,7 @@

#endif

-#endif +const char *efi_get_basename(void) +{

•   return BOOTEFI_NAME;
  

+} diff --git a/include/efi.h b/include/efi.h index 84640cf7b25..1b8093bd4d3 100644 --- a/include/efi.h +++ b/include/efi.h @@ -669,4 +669,13 @@ int efi_get_mmap(struct efi_mem_desc **descp, int *sizep, uint *keyp, */ void efi_show_tables(struct efi_system_table *systab);

+/**

• • efi_get_basename() - Get the default filename to use when loading
• • E.g. this returns BOOTAA64.EFI for an aarch target
• • Return: Default EFI filename
• */

+const char *efi_get_basename(void);

#endif /* _LINUX_EFI_H */ diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index a3aa2b8d1b9..cb9664c91a2 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -11,10 +11,10 @@ #include <blkmap.h> #include <charset.h> #include <dm.h> +#include <efi.h> #include <log.h> #include <malloc.h> #include <net.h> -#include <efi_default_filename.h> #include <efi_loader.h> #include <efi_variable.h> #include <asm/unaligned.h> @@ -82,8 +82,12 @@ struct efi_device_path *expand_media_path(struct efi_device_path *device_path) &efi_simple_file_system_protocol_guid, &rem); if (handle) { if (rem->type == DEVICE_PATH_TYPE_END) {

•                   full_path = efi_dp_from_file(device_path,
  

•                                                "/EFI/BOOT/" BOOTEFI_NAME);
  

•                   char fname[30];
  

•                   snprintf(fname, sizeof(fname), "/EFI/BOOT/%s",
  

•                            efi_get_basename());
  

•                   full_path = efi_dp_from_file(device_path, fname);
  

•           } else {
                    full_path = efi_dp_dup(device_path);
            }
  

diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index cc894e804a4..539abe63ebe 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -12,7 +12,7 @@ #include <bootstd.h> #include <cli.h> #include <dm.h> -#include <efi_default_filename.h> +#include <efi.h> #include <expo.h> #ifdef CONFIG_SANDBOX #include <asm/test.h> @@ -184,8 +184,8 @@ static int bootflow_cmd_scan_e(struct unit_test_state *uts) ut_assert_nextline(" 3 efi media mmc 0 mmc1.bootdev.whole "); ut_assert_nextline(" ** No partition found, err=-2: No such file or directory"); ut_assert_nextline(" 4 extlinux ready mmc 1 mmc1.bootdev.part_1 /extlinux/extlinux.conf");

•   ut_assert_nextline("  5  efi          fs      mmc          1  mmc1.bootdev.part_1       /EFI/BOOT/"
  

•                      BOOTEFI_NAME);
  

•   ut_assert_nextline("  5  efi          fs      mmc          1  mmc1.bootdev.part_1       /EFI/BOOT/%s",
  

•                      efi_get_basename());
  
    ut_assert_skip_to_line("Scanning bootdev 'mmc0.bootdev':");
    ut_assert_skip_to_line(
  

-- 2.43.0

Thanks /Ilias

On 10/22/24 14:00, Simon Glass wrote:

Sandbox is its own architecture, but sometimes we want to mimic the host architecture, e.g. when running an EFI app not built by U-Boot.

Add a -N/--native flag which tells sandbox to reflect the architecture of the host.

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v8:

• Add new patch to control on-host behaviour

  arch/sandbox/cpu/start.c | 10 ++++++++++ arch/sandbox/include/asm/state.h | 1 + 2 files changed, 11 insertions(+)

diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c index 81752edc9f8..2940768cd1c 100644 --- a/arch/sandbox/cpu/start.c +++ b/arch/sandbox/cpu/start.c @@ -449,6 +449,16 @@ static void setup_ram_buf(struct sandbox_state *state) gd->ram_size = state->ram_size; }

+static int sandbox_cmdline_cb_native(struct sandbox_state *state,

• 		     const char *arg)
  

+{

• state->native = true;
• return 0;

+} +SANDBOX_CMDLINE_OPT_SHORT(native, 'N', 0,

• 	  "Use native mode (host-based EFI boot filename)");
  

We should not need an option for default behavior.

Please, turn the logic around to avoid users seeing unexpected results:

+SANDBOX_CMDLINE_OPT_SHORT(non-compliant, 'N', 0,

• 	  "Non-compliant sandbox, for testing only");
  

Best regards

Heinrich

• void state_show(struct sandbox_state *state) { char **p;

diff --git a/arch/sandbox/include/asm/state.h b/arch/sandbox/include/asm/state.h index e7dc01759e8..dc21a623106 100644 --- a/arch/sandbox/include/asm/state.h +++ b/arch/sandbox/include/asm/state.h @@ -101,6 +101,7 @@ struct sandbox_state { bool disable_eth; /* Disable Ethernet devices */ bool disable_sf_bootdevs; /* Don't bind SPI flash bootdevs */ bool upl; /* Enable Universal Payload (UPL) */

• bool native; /* Adjust to reflect host arch */

  /* Pointer to information for each SPI bus/cs */ struct sandbox_spi_info spi[CONFIG_SANDBOX_SPI_MAX_BUS]

Hi Simon,

On Tue, 22 Oct 2024 at 15:00, Simon Glass sjg@chromium.org wrote:

When the --native flag is given, pretend to be running the host architecture rather than sandbox.

Add an 'efidebug filename' command to report it.

Heinrich does this allow you to continue your testing in native archs?

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v8:

• Add new patch to report host default-filename in native mode

boot/bootmeth_efi.c | 25 ++------------------ boot/efi_fname.c | 57 +++++++++++++++++++++++++++++++++------------ cmd/efidebug.c | 25 ++++++++++++++++++++ include/efi.h | 25 ++++++++++++++++++++ 4 files changed, 94 insertions(+), 38 deletions(-)

diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c index 371b36d550b..f836aa655f5 100644 --- a/boot/bootmeth_efi.c +++ b/boot/bootmeth_efi.c @@ -25,32 +25,11 @@

#define EFI_DIRNAME "/EFI/BOOT/"

-static int get_efi_pxe_arch(void) -{

•   /* http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml */
  

•   if (IS_ENABLED(CONFIG_ARM64))
  

•           return 0xb;
  

•   else if (IS_ENABLED(CONFIG_ARM))
  

•           return 0xa;
  

•   else if (IS_ENABLED(CONFIG_X86_64))
  

•           return 0x6;
  

•   else if (IS_ENABLED(CONFIG_X86))
  

•           return 0x7;
  

•   else if (IS_ENABLED(CONFIG_ARCH_RV32I))
  

•           return 0x19;
  

•   else if (IS_ENABLED(CONFIG_ARCH_RV64I))
  

•           return 0x1b;
  

•   else if (IS_ENABLED(CONFIG_SANDBOX))
  

•           return 0;       /* not used */
  

•   return -EINVAL;
  

-}

static int get_efi_pxe_vci(char *str, int max_len) { int ret;

•   ret = get_efi_pxe_arch();
  

•   ret = efi_get_pxe_arch();
    if (ret < 0)
            return ret;
  
  

@@ -239,7 +218,7 @@ static int distro_efi_read_bootflow_net(struct bootflow *bflow) ret = get_efi_pxe_vci(str, sizeof(str)); if (ret) return log_msg_ret("vci", ret);

•   ret = get_efi_pxe_arch();
  

•   ret = efi_get_pxe_arch();
    if (ret < 0)
            return log_msg_ret("arc", ret);
    arch = ret;
  

diff --git a/boot/efi_fname.c b/boot/efi_fname.c index a6b11383bba..790f9e2fa36 100644 --- a/boot/efi_fname.c +++ b/boot/efi_fname.c @@ -9,29 +9,34 @@ */

#include <efi.h> +#include <errno.h> #include <host_arch.h>

-#ifdef CONFIG_SANDBOX

#if HOST_ARCH == HOST_ARCH_X86_64 -#define BOOTEFI_NAME "BOOTX64.EFI" +#define HOST_BOOTEFI_NAME "BOOTX64.EFI" +#define HOST_PXE_ARCH 0x6 #elif HOST_ARCH == HOST_ARCH_X86 -#define BOOTEFI_NAME "BOOTIA32.EFI" +#define HOST_BOOTEFI_NAME "BOOTIA32.EFI" +#define HOST_PXE_ARCH 0x7 #elif HOST_ARCH == HOST_ARCH_AARCH64 -#define BOOTEFI_NAME "BOOTAA64.EFI" +#define HOST_BOOTEFI_NAME "BOOTAA64.EFI" +#define HOST_PXE_ARCH 0xb #elif HOST_ARCH == HOST_ARCH_ARM -#define BOOTEFI_NAME "BOOTARM.EFI" +#define HOST_BOOTEFI_NAME "BOOTARM.EFI" +#define HOST_PXE_ARCH 0xa #elif HOST_ARCH == HOST_ARCH_RISCV32 -#define BOOTEFI_NAME "BOOTRISCV32.EFI" +#define HOST_BOOTEFI_NAME "BOOTRISCV32.EFI" +#define HOST_PXE_ARCH 0x19 #elif HOST_ARCH == HOST_ARCH_RISCV64 -#define BOOTEFI_NAME "BOOTRISCV64.EFI" +#define HOST_BOOTEFI_NAME "BOOTRISCV64.EFI" +#define HOST_PXE_ARCH 0x1b #else -#error Unsupported UEFI architecture +#error Unsupported Host architecture #endif

-#else

-#if defined(CONFIG_ARM64) +#if defined(CONFIG_SANDBOX) +#define BOOTEFI_NAME "BOOTSBOX.EFI" +#elif defined(CONFIG_ARM64) #define BOOTEFI_NAME "BOOTAA64.EFI" #elif defined(CONFIG_ARM) #define BOOTEFI_NAME "BOOTARM.EFI" @@ -47,9 +52,31 @@ #error Unsupported UEFI architecture #endif

-#endif

const char *efi_get_basename(void) {

•   return BOOTEFI_NAME;
  

•   return efi_use_host_arch() ? HOST_BOOTEFI_NAME : BOOTEFI_NAME;
  

+}

+int efi_get_pxe_arch(void) +{

•   if (efi_use_host_arch())
  

•           return HOST_PXE_ARCH;
  

•   /* http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml */
  

•   if (IS_ENABLED(CONFIG_ARM64))
  

•           return 0xb;
  

•   else if (IS_ENABLED(CONFIG_ARM))
  

•           return 0xa;
  

•   else if (IS_ENABLED(CONFIG_X86_64))
  

•           return 0x6;
  

•   else if (IS_ENABLED(CONFIG_X86))
  

•           return 0x7;
  

•   else if (IS_ENABLED(CONFIG_ARCH_RV32I))
  

•           return 0x19;
  

•   else if (IS_ENABLED(CONFIG_ARCH_RV64I))
  

•           return 0x1b;
  

•   else if (IS_ENABLED(CONFIG_SANDBOX))
  

•           return 0;       /* not used */
  

•   return -EINVAL;
  

} diff --git a/cmd/efidebug.c b/cmd/efidebug.c index e040fe75fa1..02f1e080e88 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -511,6 +511,27 @@ static int do_efi_show_images(struct cmd_tbl *cmdtp, int flag, return CMD_RET_SUCCESS; }

+/**

• • do_efi_show_defaults() - show UEFI default filename and PXE architecture
• • @cmdtp: Command table
• • @flag: Command flag
• • @argc: Number of arguments
• • @argv: Argument array
• • Return: CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure
• • Implement efidebug "defaults" sub-command.
• • Shows the default EFI filename and PXE architecture
• */

+static int do_efi_show_defaults(struct cmd_tbl *cmdtp, int flag,

•                           int argc, char *const argv[])
  

+{

•   printf("Default boot path: EFI\\BOOT\\%s\n", efi_get_basename());
  

•   printf("PXE arch: 0x%02x\n", efi_get_pxe_arch());
  

•   return CMD_RET_SUCCESS;
  

+}

static const char * const efi_mem_type_string[] = { [EFI_RESERVED_MEMORY_TYPE] = "RESERVED", [EFI_LOADER_CODE] = "LOADER CODE", @@ -1561,6 +1582,8 @@ static struct cmd_tbl cmd_efidebug_sub[] = { "", ""), U_BOOT_CMD_MKENT(dh, CONFIG_SYS_MAXARGS, 1, do_efi_show_handles, "", ""),

•   U_BOOT_CMD_MKENT(defaults, CONFIG_SYS_MAXARGS, 1, do_efi_show_defaults,
  

•                    "", ""),
    U_BOOT_CMD_MKENT(images, CONFIG_SYS_MAXARGS, 1, do_efi_show_images,
                     "", ""),
    U_BOOT_CMD_MKENT(memmap, CONFIG_SYS_MAXARGS, 1, do_efi_show_memmap,
  

@@ -1653,6 +1676,8 @@ U_BOOT_LONGHELP(efidebug, " - show UEFI drivers\n" "efidebug dh\n" " - show UEFI handles\n"

•   "efidebug defaults\n"
  

•   "  - show default EFI filename and PXE architecture\n"
    "efidebug images\n"
    "  - show loaded images\n"
    "efidebug memmap\n"
  

diff --git a/include/efi.h b/include/efi.h index 1b8093bd4d3..70bb47e742f 100644 --- a/include/efi.h +++ b/include/efi.h @@ -678,4 +678,29 @@ void efi_show_tables(struct efi_system_table *systab); */ const char *efi_get_basename(void);

+#ifdef CONFIG_SANDBOX +#include <asm/state.h> +#endif

+static inline bool efi_use_host_arch(void) +{ +#ifdef CONFIG_SANDBOX

#if IS_ENABLED(CONFIG_SANDBOX) is preferred no ?

•   struct sandbox_state *state = state_get_current();
  

•   return state->native;
  

+#else

•   return false;
  

+#endif +}

+/**

• • efi_get_pxe_arch() - Get the architecture value for PXE
• • See:
• • http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml
• • Return: Architecture value
• */

+int efi_get_pxe_arch(void);

#endif /* _LINUX_EFI_H */

2.43.0

Thanks /Ilias

On 10/27/24 18:16, Simon Glass wrote:

Hi Ilias, Heinrich,

On Fri, 25 Oct 2024 at 11:57, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

...

Hi Simon,

On Tue, 22 Oct 2024 at 15:00, Simon Glass sjg@chromium.org wrote:

...

When the --native flag is given, pretend to be running the host architecture rather than sandbox.

Add an 'efidebug filename' command to report it.

Heinrich does this allow you to continue your testing in native archs?

Yes, Heinrich seems OK with it.

I highlighted to Simon, that it is important to me that the sandbox continues to support booting standard compliant images via shim, GRUB, just into the Linux EFI stub.

Introducing a sandbox specific EFI binary name or PXE value provides no value. But at least the current suggestion does not do harm except forcing sandbox users to remember another command line parameter.

Tests on QEMU or real hardware should use the architecture specific filename for the EFI binary and this is not what the test provided by patch 7/8 does.

Best regards

Heinrich

Separately from this patch, at present something is broken / missing on x86_64 (with recent QEMU / Tianocore), so I cannot run the tests with qemu-x86_64 today. It was apparently running OK before the last EDK II and QEMU updates.

Also sandbox only works with the EFI Self-Certification Test (SCT) when natively compiled on aarch64 or riscv64[1][2]. There might be something missing on sandbox x86_64, which gives a segfault when running InstallX64.efi

I am running these: ii ovmf 2024.02-2 all UEFI firmware for 64-bit x86 virtual machines ii qemu-system-x86 1:8.2.2+ds-0ubuntu1.2 amd64 QEMU full system emulation binaries (x86)

So far I have not tried going back to older versions.

...

...

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v8:

• Add new patch to report host default-filename in native mode

  boot/bootmeth_efi.c | 25 ++------------------ boot/efi_fname.c | 57 +++++++++++++++++++++++++++++++++------------ cmd/efidebug.c | 25 ++++++++++++++++++++ include/efi.h | 25 ++++++++++++++++++++ 4 files changed, 94 insertions(+), 38 deletions(-)

[..]

...

...

diff --git a/include/efi.h b/include/efi.h index 1b8093bd4d3..70bb47e742f 100644 --- a/include/efi.h +++ b/include/efi.h @@ -678,4 +678,29 @@ void efi_show_tables(struct efi_system_table *systab); */ const char *efi_get_basename(void);

+#ifdef CONFIG_SANDBOX +#include <asm/state.h> +#endif

+static inline bool efi_use_host_arch(void) +{ +#ifdef CONFIG_SANDBOX

#if IS_ENABLED(CONFIG_SANDBOX) is preferred no ?

That is more for use with if(), which I cannot use here.

...

...

•   struct sandbox_state *state = state_get_current();
  

•   return state->native;
  

+#else

•   return false;
  

+#endif +}

[..]

Regards, Simon

[1] https://github.com/U-Boot-EFI/u-boot-sct-results [2] https://github.com/xypron/sct_release_test/tree/main

Hi Heinrich,

On Mon, 28 Oct 2024 at 20:16, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 10/25/24 11:56, Ilias Apalodimas wrote:

...

Hi Simon,

On Tue, 22 Oct 2024 at 15:00, Simon Glass sjg@chromium.org wrote:

...

When the --native flag is given, pretend to be running the host architecture rather than sandbox.

Add an 'efidebug filename' command to report it.

Heinrich does this allow you to continue your testing in native archs?

...

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v8:

• Add new patch to report host default-filename in native mode

  boot/bootmeth_efi.c | 25 ++------------------ boot/efi_fname.c | 57 +++++++++++++++++++++++++++++++++------------ cmd/efidebug.c | 25 ++++++++++++++++++++ include/efi.h | 25 ++++++++++++++++++++ 4 files changed, 94 insertions(+), 38 deletions(-)

diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c index 371b36d550b..f836aa655f5 100644 --- a/boot/bootmeth_efi.c +++ b/boot/bootmeth_efi.c @@ -25,32 +25,11 @@

#define EFI_DIRNAME "/EFI/BOOT/"

-static int get_efi_pxe_arch(void) -{

•   /* http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml */
  

•   if (IS_ENABLED(CONFIG_ARM64))
  

•           return 0xb;
  

•   else if (IS_ENABLED(CONFIG_ARM))
  

•           return 0xa;
  

•   else if (IS_ENABLED(CONFIG_X86_64))
  

•           return 0x6;
  

•   else if (IS_ENABLED(CONFIG_X86))
  

•           return 0x7;
  

•   else if (IS_ENABLED(CONFIG_ARCH_RV32I))
  

•           return 0x19;
  

•   else if (IS_ENABLED(CONFIG_ARCH_RV64I))
  

•           return 0x1b;
  

•   else if (IS_ENABLED(CONFIG_SANDBOX))
  

•           return 0;       /* not used */
  

•   return -EINVAL;
  

-}

• static int get_efi_pxe_vci(char *str, int max_len) { int ret;

•   ret = get_efi_pxe_arch();
  

•   ret = efi_get_pxe_arch();
     if (ret < 0)
             return ret;
  
  

@@ -239,7 +218,7 @@ static int distro_efi_read_bootflow_net(struct bootflow *bflow) ret = get_efi_pxe_vci(str, sizeof(str)); if (ret) return log_msg_ret("vci", ret);

•   ret = get_efi_pxe_arch();
  

•   ret = efi_get_pxe_arch();
     if (ret < 0)
             return log_msg_ret("arc", ret);
     arch = ret;
  

diff --git a/boot/efi_fname.c b/boot/efi_fname.c index a6b11383bba..790f9e2fa36 100644 --- a/boot/efi_fname.c +++ b/boot/efi_fname.c @@ -9,29 +9,34 @@ */

#include <efi.h> +#include <errno.h> #include <host_arch.h>

-#ifdef CONFIG_SANDBOX

• #if HOST_ARCH == HOST_ARCH_X86_64

-#define BOOTEFI_NAME "BOOTX64.EFI" +#define HOST_BOOTEFI_NAME "BOOTX64.EFI" +#define HOST_PXE_ARCH 0x6 #elif HOST_ARCH == HOST_ARCH_X86 -#define BOOTEFI_NAME "BOOTIA32.EFI" +#define HOST_BOOTEFI_NAME "BOOTIA32.EFI" +#define HOST_PXE_ARCH 0x7 #elif HOST_ARCH == HOST_ARCH_AARCH64 -#define BOOTEFI_NAME "BOOTAA64.EFI" +#define HOST_BOOTEFI_NAME "BOOTAA64.EFI" +#define HOST_PXE_ARCH 0xb #elif HOST_ARCH == HOST_ARCH_ARM -#define BOOTEFI_NAME "BOOTARM.EFI" +#define HOST_BOOTEFI_NAME "BOOTARM.EFI" +#define HOST_PXE_ARCH 0xa #elif HOST_ARCH == HOST_ARCH_RISCV32 -#define BOOTEFI_NAME "BOOTRISCV32.EFI" +#define HOST_BOOTEFI_NAME "BOOTRISCV32.EFI" +#define HOST_PXE_ARCH 0x19 #elif HOST_ARCH == HOST_ARCH_RISCV64 -#define BOOTEFI_NAME "BOOTRISCV64.EFI" +#define HOST_BOOTEFI_NAME "BOOTRISCV64.EFI" +#define HOST_PXE_ARCH 0x1b #else -#error Unsupported UEFI architecture +#error Unsupported Host architecture #endif

-#else

-#if defined(CONFIG_ARM64) +#if defined(CONFIG_SANDBOX) +#define BOOTEFI_NAME "BOOTSBOX.EFI" +#elif defined(CONFIG_ARM64) #define BOOTEFI_NAME "BOOTAA64.EFI" #elif defined(CONFIG_ARM) #define BOOTEFI_NAME "BOOTARM.EFI" @@ -47,9 +52,31 @@ #error Unsupported UEFI architecture #endif>> -#endif

• const char *efi_get_basename(void) {

•   return BOOTEFI_NAME;
  

•   return efi_use_host_arch() ? HOST_BOOTEFI_NAME : BOOTEFI_NAME;
  

+}

+int efi_get_pxe_arch(void) +{

•   if (efi_use_host_arch())
  

•           return HOST_PXE_ARCH;
  

•   /* http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml */
  

•   if (IS_ENABLED(CONFIG_ARM64))
  

•           return 0xb;
  

•   else if (IS_ENABLED(CONFIG_ARM))
  

•           return 0xa;
  

•   else if (IS_ENABLED(CONFIG_X86_64))
  

•           return 0x6;
  

•   else if (IS_ENABLED(CONFIG_X86))
  

•           return 0x7;
  

•   else if (IS_ENABLED(CONFIG_ARCH_RV32I))
  

•           return 0x19;
  

•   else if (IS_ENABLED(CONFIG_ARCH_RV64I))
  

•           return 0x1b;
  

•   else if (IS_ENABLED(CONFIG_SANDBOX))
  

Please, add a warning here:

  log_warn("You are using the sandbox in non-compliant mode\n");

The riscv64 sandbox can only run riscv64 binaries The arm64 sandbox can only run arm64 binary.

We cannot expect a DHCP server to do the expected with the value 0.

So why should we introduce this?

...

...

•           return 0;       /* not used */
  

The following comment would fit better:

 /* This breaks PXE booting on the sandbox.*/

This line is already in the code. I am just moving it to a new file. PXE booting is not supported on sandbox. Neither is EFI booting, until this series is applied.

If you can see your way to drop your other objections on this series, I don't mind removing this line and just returning the error code.

Regards, Simon

Hi Tom,

On Tue, 29 Oct 2024 at 17:40, Tom Rini trini@konsulko.com wrote:

On Tue, Oct 29, 2024 at 04:45:54PM +0100, Simon Glass wrote:

...

Hi Heinrich,

On Mon, 28 Oct 2024 at 20:16, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 10/25/24 11:56, Ilias Apalodimas wrote:

...

Hi Simon,

On Tue, 22 Oct 2024 at 15:00, Simon Glass sjg@chromium.org wrote:

...

When the --native flag is given, pretend to be running the host architecture rather than sandbox.

Add an 'efidebug filename' command to report it.

Heinrich does this allow you to continue your testing in native archs?

...

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v8:

• Add new patch to report host default-filename in native mode

  boot/bootmeth_efi.c | 25 ++------------------ boot/efi_fname.c | 57 +++++++++++++++++++++++++++++++++------------ cmd/efidebug.c | 25 ++++++++++++++++++++ include/efi.h | 25 ++++++++++++++++++++ 4 files changed, 94 insertions(+), 38 deletions(-)

diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c index 371b36d550b..f836aa655f5 100644 --- a/boot/bootmeth_efi.c +++ b/boot/bootmeth_efi.c @@ -25,32 +25,11 @@

#define EFI_DIRNAME "/EFI/BOOT/"

-static int get_efi_pxe_arch(void) -{

•   /* http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml */
  

•   if (IS_ENABLED(CONFIG_ARM64))
  

•           return 0xb;
  

•   else if (IS_ENABLED(CONFIG_ARM))
  

•           return 0xa;
  

•   else if (IS_ENABLED(CONFIG_X86_64))
  

•           return 0x6;
  

•   else if (IS_ENABLED(CONFIG_X86))
  

•           return 0x7;
  

•   else if (IS_ENABLED(CONFIG_ARCH_RV32I))
  

•           return 0x19;
  

•   else if (IS_ENABLED(CONFIG_ARCH_RV64I))
  

•           return 0x1b;
  

•   else if (IS_ENABLED(CONFIG_SANDBOX))
  

•           return 0;       /* not used */
  

•   return -EINVAL;
  

-}

• static int get_efi_pxe_vci(char *str, int max_len) { int ret;

•   ret = get_efi_pxe_arch();
  

•   ret = efi_get_pxe_arch();
     if (ret < 0)
             return ret;
  
  

@@ -239,7 +218,7 @@ static int distro_efi_read_bootflow_net(struct bootflow *bflow) ret = get_efi_pxe_vci(str, sizeof(str)); if (ret) return log_msg_ret("vci", ret);

•   ret = get_efi_pxe_arch();
  

•   ret = efi_get_pxe_arch();
     if (ret < 0)
             return log_msg_ret("arc", ret);
     arch = ret;
  

diff --git a/boot/efi_fname.c b/boot/efi_fname.c index a6b11383bba..790f9e2fa36 100644 --- a/boot/efi_fname.c +++ b/boot/efi_fname.c @@ -9,29 +9,34 @@ */

#include <efi.h> +#include <errno.h> #include <host_arch.h>

-#ifdef CONFIG_SANDBOX

• #if HOST_ARCH == HOST_ARCH_X86_64

-#define BOOTEFI_NAME "BOOTX64.EFI" +#define HOST_BOOTEFI_NAME "BOOTX64.EFI" +#define HOST_PXE_ARCH 0x6 #elif HOST_ARCH == HOST_ARCH_X86 -#define BOOTEFI_NAME "BOOTIA32.EFI" +#define HOST_BOOTEFI_NAME "BOOTIA32.EFI" +#define HOST_PXE_ARCH 0x7 #elif HOST_ARCH == HOST_ARCH_AARCH64 -#define BOOTEFI_NAME "BOOTAA64.EFI" +#define HOST_BOOTEFI_NAME "BOOTAA64.EFI" +#define HOST_PXE_ARCH 0xb #elif HOST_ARCH == HOST_ARCH_ARM -#define BOOTEFI_NAME "BOOTARM.EFI" +#define HOST_BOOTEFI_NAME "BOOTARM.EFI" +#define HOST_PXE_ARCH 0xa #elif HOST_ARCH == HOST_ARCH_RISCV32 -#define BOOTEFI_NAME "BOOTRISCV32.EFI" +#define HOST_BOOTEFI_NAME "BOOTRISCV32.EFI" +#define HOST_PXE_ARCH 0x19 #elif HOST_ARCH == HOST_ARCH_RISCV64 -#define BOOTEFI_NAME "BOOTRISCV64.EFI" +#define HOST_BOOTEFI_NAME "BOOTRISCV64.EFI" +#define HOST_PXE_ARCH 0x1b #else -#error Unsupported UEFI architecture +#error Unsupported Host architecture #endif

-#else

-#if defined(CONFIG_ARM64) +#if defined(CONFIG_SANDBOX) +#define BOOTEFI_NAME "BOOTSBOX.EFI" +#elif defined(CONFIG_ARM64) #define BOOTEFI_NAME "BOOTAA64.EFI" #elif defined(CONFIG_ARM) #define BOOTEFI_NAME "BOOTARM.EFI" @@ -47,9 +52,31 @@ #error Unsupported UEFI architecture #endif>> -#endif

• const char *efi_get_basename(void) {

•   return BOOTEFI_NAME;
  

•   return efi_use_host_arch() ? HOST_BOOTEFI_NAME : BOOTEFI_NAME;
  

+}

+int efi_get_pxe_arch(void) +{

•   if (efi_use_host_arch())
  

•           return HOST_PXE_ARCH;
  

•   /* http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml */
  

•   if (IS_ENABLED(CONFIG_ARM64))
  

•           return 0xb;
  

•   else if (IS_ENABLED(CONFIG_ARM))
  

•           return 0xa;
  

•   else if (IS_ENABLED(CONFIG_X86_64))
  

•           return 0x6;
  

•   else if (IS_ENABLED(CONFIG_X86))
  

•           return 0x7;
  

•   else if (IS_ENABLED(CONFIG_ARCH_RV32I))
  

•           return 0x19;
  

•   else if (IS_ENABLED(CONFIG_ARCH_RV64I))
  

•           return 0x1b;
  

•   else if (IS_ENABLED(CONFIG_SANDBOX))
  

Please, add a warning here:

  log_warn("You are using the sandbox in non-compliant mode\n");

The riscv64 sandbox can only run riscv64 binaries The arm64 sandbox can only run arm64 binary.

We cannot expect a DHCP server to do the expected with the value 0.

So why should we introduce this?

...

...

•           return 0;       /* not used */
  

The following comment would fit better:

 /* This breaks PXE booting on the sandbox.*/

This line is already in the code. I am just moving it to a new file. PXE booting is not supported on sandbox. Neither is EFI booting, until this series is applied.

It sounds like EFI booting in sandbox is working and being tested and used (as a testing mechanism) right now. And the work and questions being raised here are around having your changes not break that existing use.

Well it would be, although at present that flow does not work, due to a problem either with EDK2 or QEMU. It may be fixed at some point.

With this patch, the -N flag is needed to use this 'native' flow. The flag is not needed for CI, which is what I consider to be the normal 'sandbox' case.

Regards, Simon

On 10/22/24 14:00, Simon Glass wrote:

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

---

Here is the patch to avoid sudo and CI fallback:

[1] https://patchwork.ozlabs.org/project/uboot/patch/ 20240802093322.15240-1-richard@nod.at/

(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 9bf44ae3b0b..f40fd5a2b16 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -1512,7 +1512,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 369105ca4cf..c892854b227 100644 --- a/test/boot/bootdev.c +++ b/test/boot/bootdev.c @@ -221,6 +221,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);

@@ -260,7 +264,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);

@@ -322,6 +330,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);

@@ -339,6 +351,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 539abe63ebe..d94c07963d5 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -533,7 +533,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

We should not accept such binaries. This was just how xz was hacked (https://en.wikipedia.org/wiki/XZ_Utils_backdoor).

This binary wouldn't conform to the GPL 2.0 license either.

Best regards

Heinrich

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

Hi Heinrich,

On Mon, 28 Oct 2024 at 11:25, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 10/28/24 07:08, Heinrich Schuchardt wrote:

...

On 10/22/24 14:00, Simon Glass wrote:

...

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

---

Here is the patch to avoid sudo and CI fallback:

[1] https://patchwork.ozlabs.org/project/uboot/patch/ 20240802093322.15240-1-richard@nod.at/

(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 9bf44ae3b0b..f40fd5a2b16 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -1512,7 +1512,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 369105ca4cf..c892854b227 100644 --- a/test/boot/bootdev.c +++ b/test/boot/bootdev.c @@ -221,6 +221,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);
  

@@ -260,7 +264,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);

@@ -322,6 +330,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);
  

@@ -339,6 +351,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 539abe63ebe..d94c07963d5 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -533,7 +533,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

...

We should not accept such binaries. This was just how xz was hacked (https://en.wikipedia.org/wiki/XZ_Utils_backdoor).

This binary wouldn't conform to the GPL 2.0 license either.

Yes, I've offered to tidy up the patch and drop this binary (and the others) once this test lands.

[..]

...

...

+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')
  

As said I can live with the -n switch though that standards defying bootsbox.efi name provides no benefit whatsoever. But, please, do not ignore the switch.

The -N flag is provided for allowing sandbox to use the correct, default name for the underlying host. It is not intended for use in CI, where 'sandbox is sandbox'. So far, CI calls sandbox without that flag, so the code above is actually correct.

Regards, Simon

On Mon, Oct 28, 2024 at 08:33:04PM +0100, Heinrich Schuchardt wrote:

On 10/28/24 18:02, Simon Glass wrote:

...

Hi Heinrich,

On Mon, 28 Oct 2024 at 11:25, Heinrich Schuchardt <xypron.glpk@gmx.de mailto:xypron.glpk@gmx.de> wrote:

...

On 10/28/24 07:08, Heinrich Schuchardt wrote:

...

On 10/22/24 14:00, Simon Glass wrote:

...

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

mailto:sjg@chromium.org>

...

...

...

---

Here is the patch to avoid sudo and CI fallback:

[1] https://patchwork.ozlabs.org/project/uboot/patch/ <https://

patchwork.ozlabs.org/project/uboot/patch/>

...

...

...

20240802093322.15240-1-richard@nod.at/

http://20240802093322.15240-1-richard@nod.at/

...

...

...

(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 9bf44ae3b0b..f40fd5a2b16 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -1512,7 +1512,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 369105ca4cf..c892854b227 100644 --- a/test/boot/bootdev.c +++ b/test/boot/bootdev.c @@ -221,6 +221,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); @@ -260,7 +264,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); @@ -322,6 +330,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); @@ -339,6 +351,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 539abe63ebe..d94c07963d5 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -533,7 +533,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

...

...

We should not accept such binaries. This was just how xz was hacked (https://en.wikipedia.org/wiki/XZ_Utils_backdoor <https://

en.wikipedia.org/wiki/XZ_Utils_backdoor>).

...

...

This binary wouldn't conform to the GPL 2.0 license either.

Yes, I've offered to tidy up the patch and drop this binary (and the others) once this test lands.

[..]

...

...

...

+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')

As said I can live with the -n switch though that standards defying bootsbox.efi name provides no benefit whatsoever. But, please, do not ignore the switch.

The -N flag is provided for allowing sandbox to use the correct, default name for the underlying host. It is not intended for use in CI, where 'sandbox is sandbox'. So far, CI calls sandbox without that flag, so the code above is actually correct.

Regards, Simon

Hello Simon,

We are in round 8 of this patch series and it is still broken:

You supply an xz image which we should never accept for security reasons.

That xz image contains an EFI/BOOT/BOOTSBOX.EFI blob. This does not comply to the GPLv2 license.

That blob is an amd64 binary. So nobody will be able it to run it in the sandbox built on the riscv64 or arm64 machines.

• Please, remove the blob.
• Do the same for all blobs in test/py/tests/bootstd/.
• Provide a test test runs on all EFI architectures.
• Make sure that we can run the test both on QEMU and on real hardware.
• Please, comply in your test to the UEFI specification.

Simon has said he will pick up and finish off the series that removes requiring sudo for tests, and I'm OK with that being a follow-up cleanup to this series. It's not a new problem, and since he's said he'll fix it, I'm OK with that.