On 02.09.24 03:18, Simon Glass wrote:

Sandbox is not a real architecture, but within U-Boot it is real enough. We should not need to pretend it is x86 or ARM anywhere in the code.

Also we want to be able to locate the sandbox app using a single filename, 'bootsbox.efi', to avoid needing tests to produce different files on each host architecture.

The changed include is not meant to set anything for the sandbox app which you run on top of an UEFI firmware. The sandbox app probably should be called depending on the architecture sandboxaa64.efi, sandobxriscv64.efi, etc.

This include is defining which file the EFI boot manager loads by default. This value must conform to the EFI specification. When I have built the sandbox on riscv64 I expect that it will load bootriscv64.efi from a distro installer image.

Best regards

Heinrich

Drop the confusing use of host architecture and just let sandbox be sandbox.

Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

---

Changes in v5:

• Drop the Fixes tag

Changes in v3:

• Put back the Linaro copyright accidentally removed

  include/efi_default_filename.h | 24 ++---------------------- 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/include/efi_default_filename.h b/include/efi_default_filename.h index 77932984b55..06ca8735002 100644 --- a/include/efi_default_filename.h +++ b/include/efi_default_filename.h @@ -16,26 +16,8 @@ #undef BOOTEFI_NAME

#ifdef CONFIG_SANDBOX

-#if HOST_ARCH == HOST_ARCH_X86_64 -#define BOOTEFI_NAME "BOOTX64.EFI" -#elif HOST_ARCH == HOST_ARCH_X86 -#define BOOTEFI_NAME "BOOTIA32.EFI" -#elif HOST_ARCH == HOST_ARCH_AARCH64 -#define BOOTEFI_NAME "BOOTAA64.EFI" -#elif HOST_ARCH == HOST_ARCH_ARM -#define BOOTEFI_NAME "BOOTARM.EFI" -#elif HOST_ARCH == HOST_ARCH_RISCV32 -#define BOOTEFI_NAME "BOOTRISCV32.EFI" -#elif HOST_ARCH == HOST_ARCH_RISCV64 -#define BOOTEFI_NAME "BOOTRISCV64.EFI" -#else -#error Unsupported UEFI architecture -#endif

-#else

-#if defined(CONFIG_ARM64) +#define BOOTEFI_NAME "BOOTSBOX.EFI" +#elif defined(CONFIG_ARM64) #define BOOTEFI_NAME "BOOTAA64.EFI" #elif defined(CONFIG_ARM) #define BOOTEFI_NAME "BOOTARM.EFI" @@ -52,5 +34,3 @@ #endif

#endif

-#endif

Hi Heinrich,

On Thu, 12 Sept 2024 at 08:58, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 02.09.24 03:18, Simon Glass wrote:

...

We don't want ANSI characters written in tests since it is a pain to check the output with ut_assert_nextline() et al.

Provide a way to tests to request that ANSI characters not be sent.

Add a proper function comment while we are here, to encourage others.

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

This patch was already NAKed in the last version.

Please, adjust the test framework if you need filtering.

In what way should it be adjusted? I hope you are not suggesting that sandbox should try to filter out ANSI characters in its assertions?

We need our code to be designed around testing. The current code blindly sends out ANSI codes without no knowledge of whether there is anything at the other end which understands them.

Anyway, please make a concrete suggestion. At this point, at v5, I would just like this series applied ASAP with minimal extra effort.

Regards, Simon

Best regards

Heinrich

...

---

(no changes since v1)

include/efi_loader.h | 21 ++++++++++++++++++++- lib/efi_loader/efi_console.c | 26 +++++++++++++++++--------- 2 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h index f84852e384f..82b90ee0f1d 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -531,8 +531,27 @@ efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index); efi_status_t efi_bootmgr_run(void *fdt); /* search the boot option index in BootOrder */ bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *index); -/* Set up console modes */

+/**

• • efi_setup_console_size() - update the mode table.
• • By default the only mode available is 80x25. If the console has at least 50
• • lines, enable mode 80x50. If we can query the console size and it is neither
• • 80x25 nor 80x50, set it as an additional mode.
• */ void efi_setup_console_size(void);

+/**

• • efi_console_set_ansi() - Set whether ANSI characters should be emitted
• • These characters mess up tests which use ut_assert_nextline(). Call this
• • function to tell efi_loader not to emit these characters when starting up the
• • terminal
• • @allow_ansi: Allow emitting ANSI characters
• */

+void efi_console_set_ansi(bool allow_ansi);

• /* Set up load options from environment variable */ efi_status_t efi_env_set_load_options(efi_handle_t handle, const char *env_var, u16 **load_options);

diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index cea50c748aa..569fc9199bc 100644 --- a/lib/efi_loader/efi_console.c +++ b/lib/efi_loader/efi_console.c @@ -30,6 +30,17 @@ struct cout_mode {

__maybe_unused static struct efi_object uart_obj;

+/*

• • suppress emission of ANSI codes for use by unit tests. Leave it as 0 for the
• • default behaviour
• */

+static bool no_ansi;

+void efi_console_set_ansi(bool allow_ansi) +{

• no_ansi = !allow_ansi;
  

+}

• static struct cout_mode efi_cout_modes[] = { /* EFI Mode 0 is 80x25 and always present */ {

@@ -348,13 +359,6 @@ static int __maybe_unused query_vidconsole(int *rows, int *cols) return 0; }

-/**

• • efi_setup_console_size() - update the mode table.
• • By default the only mode available is 80x25. If the console has at least 50
• • lines, enable mode 80x50. If we can query the console size and it is neither
• • 80x25 nor 80x50, set it as an additional mode.
• */ void efi_setup_console_size(void) { int rows = 25, cols = 80;

@@ -362,8 +366,12 @@ void efi_setup_console_size(void)

  if (IS_ENABLED(CONFIG_VIDEO))
          ret = query_vidconsole(&rows, &cols);

• if (ret)
  

•         ret = query_console_serial(&rows, &cols);
  

• if (ret) {
  

•         if (no_ansi)
  

•                 ret = 0;
  

•         else
  

•                 ret = query_console_serial(&rows, &cols);
  

• }
  if (ret)
          return;
  
  

On 02.09.24 03:18, 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.

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

(no changes since v1)

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

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index ab2c1c44364..4de05c6f2d6 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -528,4 +528,14 @@ 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

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

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

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

+#include <efi_api.h>

+static const efi_guid_t loaded_image_guid = EFI_LOADED_IMAGE_PROTOCOL_GUID;

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

+/**

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

+efi_status_t EFIAPI efi_main(efi_handle_t handle,

• 	     struct efi_system_table *systab)
  

+{

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

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

• 		      EFI_OPEN_PROTOCOL_GET_PROTOCOL);
  

• if (ret != EFI_SUCCESS) {

• con_out->output_string
  

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

• goto out;
  

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

+out:

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

• 		       &desc_version);
  

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

You cannot call boot services after ExitBootServices().

Best regards

Heinrich

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

+}

Hi Heinrich,

On Thu, 12 Sept 2024 at 09:12, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 02.09.24 03:18, Simon Glass wrote:

...

While sandbox supports virtio it cannot support actually using the block devices to read files, since there is nothing on the other end of the 'virtqueue'.

A recent change makes EFI probe all block devices, whether used or not. This is apparently required by EFI, although it violates U-Boot's lazy-init principle.

We always did this.

Commit d5391bf02b9 dates from 2022, so I don't think that is correct.

What problem do you want to fix? I have not seen any issues in our CI.

The EFI test in this series hangs trying to probe a virtio block device. If you drop this patch and try the rest of the series in CI, you will see the failure. Or you could just accept that I investigated this, root-caused it and produced a suitable fix. This is a v5 patch which has had quite a bit of discussion.

...

We cannot just drop the virtio devices as they are used in sandbox tests.

So for now just add a special case to work around this.

The eventual fix is likely adding an implementation of virtio_sandbox_notify() to actually do the block read. That is tracked in [1].

Signed-off-by: Simon Glass sjg@chromium.org Fixes: d5391bf02b9 ("efi_loader: ensure all block devices are probed") Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org [1] https://source.denx.de/u-boot/u-boot/-/issues/37

---

(no changes since v3)

Changes in v3:

• Add a Fixes tag

• Mention the issue created for this problem

  lib/efi_loader/efi_disk.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 93a9a5ac025..2e1d37848fc 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -838,8 +838,20 @@ efi_status_t efi_disk_get_device_name(const efi_handle_t handle, char *buf, int efi_status_t efi_disks_register(void) { struct udevice *dev;

• struct uclass *uc;
  
  

• uclass_foreach_dev_probe(UCLASS_BLK, dev) {
  

• uclass_id_foreach_dev(UCLASS_BLK, dev, uc) {
  

•         /*
  

•          * The virtio block-device hangs on sandbox when accessed since
  

•          * there is nothing listening to the mailbox
  

•          */
  

•         if (IS_ENABLED(CONFIG_SANDBOX)) {
  

•                 struct blk_desc *desc = dev_get_uclass_plat(dev);
  

•                 if (desc->uclass_id == UCLASS_VIRTIO)
  

•                         continue;
  

•         }
  

•         device_probe(dev);
  }
  
  return EFI_SUCCESS;
  

Please, do not spray sandbox tweaks all over the place.

Can't you just return an error from the sandbox-virtio driver when an attempt to read a queue is made?

We are using virtio on QEMU. Why do we need sandbox virtio devices? Just run the relevant tests on the real thing.

Please go ahead with whatever approach to testing you wish. But sandbox testing is an important component of U-Boot.

Regards, Simon

Hi Tom,

On Tue, 17 Sept 2024 at 19:03, Tom Rini trini@konsulko.com wrote:

On Mon, Sep 16, 2024 at 05:42:31PM +0200, Simon Glass wrote:

...

Hi Heinrich,

On Thu, 12 Sept 2024 at 09:12, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 02.09.24 03:18, Simon Glass wrote:

...

While sandbox supports virtio it cannot support actually using the block devices to read files, since there is nothing on the other end of the 'virtqueue'.

A recent change makes EFI probe all block devices, whether used or not. This is apparently required by EFI, although it violates U-Boot's lazy-init principle.

We always did this.

Commit d5391bf02b9 dates from 2022, so I don't think that is correct.

Yes, but I also could have sworn that was fixing the behavior having been changed again previous to that.

I don't see any evidence of that, though.

...

...

What problem do you want to fix? I have not seen any issues in our CI.

The EFI test in this series hangs trying to probe a virtio block device. If you drop this patch and try the rest of the series in CI, you will see the failure. Or you could just accept that I investigated this, root-caused it and produced a suitable fix. This is a v5 patch which has had quite a bit of discussion.

And as I noted an iteration or two back, it's entirely unclear if the problem is "sandbox virtio is broken" or "this code is wrong here". Which in fact gets us back to ...

sandbox virtio does not support a functioning block device

...

...

...

We cannot just drop the virtio devices as they are used in sandbox tests.

So for now just add a special case to work around this.

The eventual fix is likely adding an implementation of virtio_sandbox_notify() to actually do the block read. That is tracked in [1].

Signed-off-by: Simon Glass sjg@chromium.org Fixes: d5391bf02b9 ("efi_loader: ensure all block devices are probed") Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org [1] https://source.denx.de/u-boot/u-boot/-/issues/37

---

(no changes since v3)

Changes in v3:

• Add a Fixes tag

• Mention the issue created for this problem

  lib/efi_loader/efi_disk.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 93a9a5ac025..2e1d37848fc 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -838,8 +838,20 @@ efi_status_t efi_disk_get_device_name(const efi_handle_t handle, char *buf, int efi_status_t efi_disks_register(void) { struct udevice *dev;

• struct uclass *uc;
  
  

• uclass_foreach_dev_probe(UCLASS_BLK, dev) {
  

• uclass_id_foreach_dev(UCLASS_BLK, dev, uc) {
  

•         /*
  

•          * The virtio block-device hangs on sandbox when accessed since
  

•          * there is nothing listening to the mailbox
  

•          */
  

•         if (IS_ENABLED(CONFIG_SANDBOX)) {
  

•                 struct blk_desc *desc = dev_get_uclass_plat(dev);
  

•                 if (desc->uclass_id == UCLASS_VIRTIO)
  

•                         continue;
  

•         }
  

•         device_probe(dev);
  }
  
  return EFI_SUCCESS;
  

Please, do not spray sandbox tweaks all over the place.

Can't you just return an error from the sandbox-virtio driver when an attempt to read a queue is made?

We are using virtio on QEMU. Why do we need sandbox virtio devices? Just run the relevant tests on the real thing.

Please go ahead with whatever approach to testing you wish. But sandbox testing is an important component of U-Boot.

Yes, but is sandbox implementing the "just be a state machine" part here correctly, or not? It keeps feeling like "not" and that the reasonable course of action would be to stop testing this on sandbox until that is fixed especially since we can test this reliably on qemu.

We have to move things forward a piece at a time. Not having a proper test for the EFI bootmeth is a significant gap and is what I am trying to fix with this series. It isn't perfect, but it is a step forward and will prevent regressions. It can also be built on later.

Happy-path testing with QEMU is all very well, but it only goes so far.

Regards, Simon

Hi Tom,

On Thu, 19 Sept 2024 at 19:45, Tom Rini trini@konsulko.com wrote:

On Thu, Sep 19, 2024 at 04:10:12PM +0200, Simon Glass wrote:

...

Hi Tom,

On Tue, 17 Sept 2024 at 19:03, Tom Rini trini@konsulko.com wrote:

...

On Mon, Sep 16, 2024 at 05:42:31PM +0200, Simon Glass wrote:

...

Hi Heinrich,

On Thu, 12 Sept 2024 at 09:12, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 02.09.24 03:18, Simon Glass wrote:

...

While sandbox supports virtio it cannot support actually using the block devices to read files, since there is nothing on the other end of the 'virtqueue'.

A recent change makes EFI probe all block devices, whether used or not. This is apparently required by EFI, although it violates U-Boot's lazy-init principle.

We always did this.

Commit d5391bf02b9 dates from 2022, so I don't think that is correct.

Yes, but I also could have sworn that was fixing the behavior having been changed again previous to that.

I don't see any evidence of that, though.

Yes, it's just my recollection of the time and I could be misremembering it as one of the other times we've had this discussion.

...

...

...

...

What problem do you want to fix? I have not seen any issues in our CI.

The EFI test in this series hangs trying to probe a virtio block device. If you drop this patch and try the rest of the series in CI, you will see the failure. Or you could just accept that I investigated this, root-caused it and produced a suitable fix. This is a v5 patch which has had quite a bit of discussion.

And as I noted an iteration or two back, it's entirely unclear if the problem is "sandbox virtio is broken" or "this code is wrong here". Which in fact gets us back to ...

sandbox virtio does not support a functioning block device

...

...

...

...

We cannot just drop the virtio devices as they are used in sandbox tests.

So for now just add a special case to work around this.

The eventual fix is likely adding an implementation of virtio_sandbox_notify() to actually do the block read. That is tracked in [1].

Signed-off-by: Simon Glass sjg@chromium.org Fixes: d5391bf02b9 ("efi_loader: ensure all block devices are probed") Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org [1] https://source.denx.de/u-boot/u-boot/-/issues/37

---

(no changes since v3)

Changes in v3:

• Add a Fixes tag

• Mention the issue created for this problem

  lib/efi_loader/efi_disk.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 93a9a5ac025..2e1d37848fc 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -838,8 +838,20 @@ efi_status_t efi_disk_get_device_name(const efi_handle_t handle, char *buf, int efi_status_t efi_disks_register(void) { struct udevice *dev;

• struct uclass *uc;
  
  

• uclass_foreach_dev_probe(UCLASS_BLK, dev) {
  

• uclass_id_foreach_dev(UCLASS_BLK, dev, uc) {
  

•         /*
  

•          * The virtio block-device hangs on sandbox when accessed since
  

•          * there is nothing listening to the mailbox
  

•          */
  

•         if (IS_ENABLED(CONFIG_SANDBOX)) {
  

•                 struct blk_desc *desc = dev_get_uclass_plat(dev);
  

•                 if (desc->uclass_id == UCLASS_VIRTIO)
  

•                         continue;
  

•         }
  

•         device_probe(dev);
  }
  
  return EFI_SUCCESS;
  

Please, do not spray sandbox tweaks all over the place.

Can't you just return an error from the sandbox-virtio driver when an attempt to read a queue is made?

We are using virtio on QEMU. Why do we need sandbox virtio devices? Just run the relevant tests on the real thing.

Please go ahead with whatever approach to testing you wish. But sandbox testing is an important component of U-Boot.

Yes, but is sandbox implementing the "just be a state machine" part here correctly, or not? It keeps feeling like "not" and that the reasonable course of action would be to stop testing this on sandbox until that is fixed especially since we can test this reliably on qemu.

OK, but I can't tell if your answer to my point here is:

• Yes, sandbox virtio is broken / incomplete
• No, sandbox virtio is fine, there's some other mismatch between how it's used for sandbox and how it's used for QEMU. This will get resolved later.

It is incomplete, in that the block device is not fully implemented. No other test enables it, but EFI does, since it blinding tries to access all block devices without any control.

...

We have to move things forward a piece at a time. Not having a proper test for the EFI bootmeth is a significant gap and is what I am trying to fix with this series. It isn't perfect, but it is a step forward and will prevent regressions. It can also be built on later.

Happy-path testing with QEMU is all very well, but it only goes so far.

I frankly get really puzzled about why testing all of this, in QEMU, where we could actually design the test to see if the OS has booted (and if we leave things configurable well enough, do this on real hardware) is wrong but sandbox, where we can't boot the OS, is good. Especially for the device that's only present in an emulator. We're emulating an emulator and not getting matching behavior in our emulator.

There are many reasons: - with sandbox we can test the operation of the bootmeth, including under failure conditions - we can test what happens within U-Boot itself when exit-boot-services is called, which is the bug that provoked me to write the test - we can build on this test to cover other bootmeths without needing to install a full OS just to run a test

Just the sheer number of bugs found and fixed by this test should show its value. One of the bugs was an access at location 0 which many boards happily accept, but sandbox catches.

Also, someone should keep track of all the bugs we never see, because a test covers them. For example, driver model is well-covered by tests and we seldom see a bug. If all we had was QEMU, then all our tests would be 'happy path' and we would be in a very different position.

One lesson I see here is that I should have pushed for a proper block device with virtio at the time. But who could have foreseen that EFI would throw away the 'lazy init' approach of U-Boot and just scan all the devices?

Regards, Simon

Hi Tom,

On Fri, 20 Sept 2024 at 16:59, Tom Rini trini@konsulko.com wrote:

On Fri, Sep 20, 2024 at 09:25:29AM +0200, Simon Glass wrote:

...

Hi Tom,

On Thu, 19 Sept 2024 at 19:45, Tom Rini trini@konsulko.com wrote:

...

On Thu, Sep 19, 2024 at 04:10:12PM +0200, Simon Glass wrote:

...

Hi Tom,

On Tue, 17 Sept 2024 at 19:03, Tom Rini trini@konsulko.com wrote:

...

On Mon, Sep 16, 2024 at 05:42:31PM +0200, Simon Glass wrote:

...

Hi Heinrich,

On Thu, 12 Sept 2024 at 09:12, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > On 02.09.24 03:18, Simon Glass wrote: > > While sandbox supports virtio it cannot support actually using the block > > devices to read files, since there is nothing on the other end of the > > 'virtqueue'. > > > > A recent change makes EFI probe all block devices, whether used or not. > > This is apparently required by EFI, although it violates U-Boot's > > lazy-init principle. > > We always did this.

Commit d5391bf02b9 dates from 2022, so I don't think that is correct.

Yes, but I also could have sworn that was fixing the behavior having been changed again previous to that.

I don't see any evidence of that, though.

Yes, it's just my recollection of the time and I could be misremembering it as one of the other times we've had this discussion.

...

...

...

> What problem do you want to fix? I have not seen any issues in our CI.

The EFI test in this series hangs trying to probe a virtio block device. If you drop this patch and try the rest of the series in CI, you will see the failure. Or you could just accept that I investigated this, root-caused it and produced a suitable fix. This is a v5 patch which has had quite a bit of discussion.

And as I noted an iteration or two back, it's entirely unclear if the problem is "sandbox virtio is broken" or "this code is wrong here". Which in fact gets us back to ...

sandbox virtio does not support a functioning block device

...

...

> > We cannot just drop the virtio devices as they are used in sandbox tests. > > > > So for now just add a special case to work around this. > > > > The eventual fix is likely adding an implementation of > > virtio_sandbox_notify() to actually do the block read. That is tracked > > in [1]. > > > > Signed-off-by: Simon Glass sjg@chromium.org > > Fixes: d5391bf02b9 ("efi_loader: ensure all block devices are probed") > > Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org > > [1] https://source.denx.de/u-boot/u-boot/-/issues/37 > > --- > > > > (no changes since v3) > > > > Changes in v3: > > - Add a Fixes tag > > - Mention the issue created for this problem > > > > lib/efi_loader/efi_disk.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > > index 93a9a5ac025..2e1d37848fc 100644 > > --- a/lib/efi_loader/efi_disk.c > > +++ b/lib/efi_loader/efi_disk.c > > @@ -838,8 +838,20 @@ efi_status_t efi_disk_get_device_name(const efi_handle_t handle, char *buf, int > > efi_status_t efi_disks_register(void) > > { > > struct udevice *dev; > > + struct uclass *uc; > > > > - uclass_foreach_dev_probe(UCLASS_BLK, dev) { > > + uclass_id_foreach_dev(UCLASS_BLK, dev, uc) { > > + /* > > + * The virtio block-device hangs on sandbox when accessed since > > + * there is nothing listening to the mailbox > > + */ > > + if (IS_ENABLED(CONFIG_SANDBOX)) { > > + struct blk_desc *desc = dev_get_uclass_plat(dev); > > + > > + if (desc->uclass_id == UCLASS_VIRTIO) > > + continue; > > + } > > + device_probe(dev); > > } > > > > return EFI_SUCCESS; > > Please, do not spray sandbox tweaks all over the place. > > Can't you just return an error from the sandbox-virtio driver when an > attempt to read a queue is made? > > We are using virtio on QEMU. Why do we need sandbox virtio devices? Just > run the relevant tests on the real thing.

Please go ahead with whatever approach to testing you wish. But sandbox testing is an important component of U-Boot.

Yes, but is sandbox implementing the "just be a state machine" part here correctly, or not? It keeps feeling like "not" and that the reasonable course of action would be to stop testing this on sandbox until that is fixed especially since we can test this reliably on qemu.

OK, but I can't tell if your answer to my point here is:

• Yes, sandbox virtio is broken / incomplete
• No, sandbox virtio is fine, there's some other mismatch between how it's used for sandbox and how it's used for QEMU. This will get resolved later.

It is incomplete, in that the block device is not fully implemented.

Then please disable it until you can complete it.

...

No other test enables it, but EFI does, since it blinding tries to access all block devices without any control.

...

...

We have to move things forward a piece at a time. Not having a proper test for the EFI bootmeth is a significant gap and is what I am trying to fix with this series. It isn't perfect, but it is a step forward and will prevent regressions. It can also be built on later.

Happy-path testing with QEMU is all very well, but it only goes so far.

I frankly get really puzzled about why testing all of this, in QEMU, where we could actually design the test to see if the OS has booted (and if we leave things configurable well enough, do this on real hardware) is wrong but sandbox, where we can't boot the OS, is good. Especially for the device that's only present in an emulator. We're emulating an emulator and not getting matching behavior in our emulator.

There are many reasons:

To be clear, I'm not saying sandbox tests have no value, or are unimportant. I apologize for imply as much.

...

• with sandbox we can test the operation of the bootmeth, including

under failure conditions

Yes, state machine tests are useful. But we can test for xFAIL on other platforms, yes?

...

• we can test what happens within U-Boot itself when

exit-boot-services is called, which is the bug that provoked me to write the test

I honestly don't recall the state of the discussion around that patch, positive/negative/neutral.

...

• we can build on this test to cover other bootmeths without needing

to install a full OS just to run a test

Counter point, we can't test that an OS actually boots. One of the most valuable personally tests we've added recently is test/py/tests/test_net_boot.py which makes the network load and boot an OS image, and test for (some) failure modes.

What do you want me to do with this patch?

Without it, the test cannot pass. Are you suggesting that we apply the patches, but don't enable the test until it can be made to work, without this patch? Are you suggesting that I implement block devices for virtio in sandbox?

I see great value in this test. It covers a case which we don't currently have in CI.

Please can you just take this patch so we can move forward?? I'm happy to add the virtio support later.

Regards, Simon

Hi Tom,

On Wed, 25 Sept 2024 at 19:26, Tom Rini trini@konsulko.com wrote:

On Wed, Sep 25, 2024 at 02:52:04PM +0200, Simon Glass wrote:

...

Hi Tom,

On Fri, 20 Sept 2024 at 16:59, Tom Rini trini@konsulko.com wrote:

...

On Fri, Sep 20, 2024 at 09:25:29AM +0200, Simon Glass wrote:

...

Hi Tom,

On Thu, 19 Sept 2024 at 19:45, Tom Rini trini@konsulko.com wrote:

...

On Thu, Sep 19, 2024 at 04:10:12PM +0200, Simon Glass wrote:

...

Hi Tom,

On Tue, 17 Sept 2024 at 19:03, Tom Rini trini@konsulko.com wrote: > > On Mon, Sep 16, 2024 at 05:42:31PM +0200, Simon Glass wrote: > > Hi Heinrich, > > > > On Thu, 12 Sept 2024 at 09:12, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > > > > > On 02.09.24 03:18, Simon Glass wrote: > > > > While sandbox supports virtio it cannot support actually using the block > > > > devices to read files, since there is nothing on the other end of the > > > > 'virtqueue'. > > > > > > > > A recent change makes EFI probe all block devices, whether used or not. > > > > This is apparently required by EFI, although it violates U-Boot's > > > > lazy-init principle. > > > > > > We always did this. > > > > Commit d5391bf02b9 dates from 2022, so I don't think that is correct. > > Yes, but I also could have sworn that was fixing the behavior having > been changed again previous to that.

I don't see any evidence of that, though.

Yes, it's just my recollection of the time and I could be misremembering it as one of the other times we've had this discussion.

...

> > > What problem do you want to fix? I have not seen any issues in our CI. > > > > The EFI test in this series hangs trying to probe a virtio block > > device. If you drop this patch and try the rest of the series in CI, > > you will see the failure. Or you could just accept that I investigated > > this, root-caused it and produced a suitable fix. This is a v5 patch > > which has had quite a bit of discussion. > > And as I noted an iteration or two back, it's entirely unclear if the > problem is "sandbox virtio is broken" or "this code is wrong here". > Which in fact gets us back to ...

sandbox virtio does not support a functioning block device

> > > > > We cannot just drop the virtio devices as they are used in sandbox tests. > > > > > > > > So for now just add a special case to work around this. > > > > > > > > The eventual fix is likely adding an implementation of > > > > virtio_sandbox_notify() to actually do the block read. That is tracked > > > > in [1]. > > > > > > > > Signed-off-by: Simon Glass sjg@chromium.org > > > > Fixes: d5391bf02b9 ("efi_loader: ensure all block devices are probed") > > > > Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org > > > > [1] https://source.denx.de/u-boot/u-boot/-/issues/37 > > > > --- > > > > > > > > (no changes since v3) > > > > > > > > Changes in v3: > > > > - Add a Fixes tag > > > > - Mention the issue created for this problem > > > > > > > > lib/efi_loader/efi_disk.c | 14 +++++++++++++- > > > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > > > > index 93a9a5ac025..2e1d37848fc 100644 > > > > --- a/lib/efi_loader/efi_disk.c > > > > +++ b/lib/efi_loader/efi_disk.c > > > > @@ -838,8 +838,20 @@ efi_status_t efi_disk_get_device_name(const efi_handle_t handle, char *buf, int > > > > efi_status_t efi_disks_register(void) > > > > { > > > > struct udevice *dev; > > > > + struct uclass *uc; > > > > > > > > - uclass_foreach_dev_probe(UCLASS_BLK, dev) { > > > > + uclass_id_foreach_dev(UCLASS_BLK, dev, uc) { > > > > + /* > > > > + * The virtio block-device hangs on sandbox when accessed since > > > > + * there is nothing listening to the mailbox > > > > + */ > > > > + if (IS_ENABLED(CONFIG_SANDBOX)) { > > > > + struct blk_desc *desc = dev_get_uclass_plat(dev); > > > > + > > > > + if (desc->uclass_id == UCLASS_VIRTIO) > > > > + continue; > > > > + } > > > > + device_probe(dev); > > > > } > > > > > > > > return EFI_SUCCESS; > > > > > > Please, do not spray sandbox tweaks all over the place. > > > > > > Can't you just return an error from the sandbox-virtio driver when an > > > attempt to read a queue is made? > > > > > > We are using virtio on QEMU. Why do we need sandbox virtio devices? Just > > > run the relevant tests on the real thing. > > > > Please go ahead with whatever approach to testing you wish. But > > sandbox testing is an important component of U-Boot. > > Yes, but is sandbox implementing the "just be a state machine" part here > correctly, or not? It keeps feeling like "not" and that the reasonable > course of action would be to stop testing this on sandbox until that is > fixed especially since we can test this reliably on qemu.

OK, but I can't tell if your answer to my point here is:

• Yes, sandbox virtio is broken / incomplete
• No, sandbox virtio is fine, there's some other mismatch between how it's used for sandbox and how it's used for QEMU. This will get resolved later.

It is incomplete, in that the block device is not fully implemented.

Then please disable it until you can complete it.

...

No other test enables it, but EFI does, since it blinding tries to access all block devices without any control.

...

...

We have to move things forward a piece at a time. Not having a proper test for the EFI bootmeth is a significant gap and is what I am trying to fix with this series. It isn't perfect, but it is a step forward and will prevent regressions. It can also be built on later.

Happy-path testing with QEMU is all very well, but it only goes so far.

I frankly get really puzzled about why testing all of this, in QEMU, where we could actually design the test to see if the OS has booted (and if we leave things configurable well enough, do this on real hardware) is wrong but sandbox, where we can't boot the OS, is good. Especially for the device that's only present in an emulator. We're emulating an emulator and not getting matching behavior in our emulator.

There are many reasons:

To be clear, I'm not saying sandbox tests have no value, or are unimportant. I apologize for imply as much.

...

• with sandbox we can test the operation of the bootmeth, including

under failure conditions

Yes, state machine tests are useful. But we can test for xFAIL on other platforms, yes?

...

• we can test what happens within U-Boot itself when

exit-boot-services is called, which is the bug that provoked me to write the test

I honestly don't recall the state of the discussion around that patch, positive/negative/neutral.

...

• we can build on this test to cover other bootmeths without needing

to install a full OS just to run a test

Counter point, we can't test that an OS actually boots. One of the most valuable personally tests we've added recently is test/py/tests/test_net_boot.py which makes the network load and boot an OS image, and test for (some) failure modes.

What do you want me to do with this patch?

Without it, the test cannot pass. Are you suggesting that we apply the patches, but don't enable the test until it can be made to work, without this patch? Are you suggesting that I implement block devices for virtio in sandbox?

I see great value in this test. It covers a case which we don't currently have in CI.

Please can you just take this patch so we can move forward?? I'm happy to add the virtio support later.

I'm suggesting, again, that you need to disable the sandbox virtio driver until you can come and complete it.

OK, I did not understand the 'it' in your previous comment. I will do that, thanks.

Regards, Simon

On 02.09.24 03:18, 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.

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

(no changes since v1)

arch/sandbox/dts/test.dts | 2 +- test/boot/bootdev.c | 18 +++++++++- test/boot/bootflow.c | 2 +- test/py/tests/bootstd/flash1.img.xz | Bin 0 -> 5016 bytes test/py/tests/test_ut.py | 52 ++++++++++++++++++++++++---- 5 files changed, 65 insertions(+), 9 deletions(-) create mode 100644 test/py/tests/bootstd/flash1.img.xz

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index 5fb5eac862e..a99de6e62ce 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -1507,7 +1507,7 @@ flash-stick@1 { reg = <1>; compatible = "sandbox,usb-flash";

• 			sandbox,filepath = "testflash1.bin";
  

• 			sandbox,filepath = "flash1.img";
  	};
  
  	flash-stick@2 {
  

diff --git a/test/boot/bootdev.c b/test/boot/bootdev.c index c635d06ec25..269bcd693a6 100644 --- a/test/boot/bootdev.c +++ b/test/boot/bootdev.c @@ -212,6 +212,10 @@ static int bootdev_test_order(struct unit_test_state *uts) /* Use the environment variable to override it */ ut_assertok(env_set("boot_targets", "mmc1 mmc2 usb")); ut_assertok(bootflow_scan_first(NULL, NULL, &iter, 0, &bflow));

• /* get the usb device which has a backing file (flash1.img) */
• ut_asserteq(0, bootflow_scan_next(&iter, &bflow));
• ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow)); ut_asserteq(5, iter.num_devs); ut_asserteq_str("mmc1.bootdev", iter.dev_used[0]->name);

@@ -251,7 +255,11 @@ static int bootdev_test_order(struct unit_test_state *uts) ut_assertok(bootflow_scan_first(NULL, NULL, &iter, 0, &bflow)); ut_asserteq(2, iter.num_devs);

• /* Now scan past mmc1 and make sure that the 3 USB devices show up */

• /*

• * Now scan past mmc1 and make sure that the 3 USB devices show up. The
  

• * first one has a backing file so returns success
  

• */
  

• ut_asserteq(0, bootflow_scan_next(&iter, &bflow)); ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow)); ut_asserteq(6, iter.num_devs); ut_asserteq_str("mmc2.bootdev", iter.dev_used[0]->name);

@@ -313,6 +321,10 @@ static int bootdev_test_prio(struct unit_test_state *uts)

/* 3 MMC and 3 USB bootdevs: MMC should come before USB */ ut_assertok(bootflow_scan_first(NULL, NULL, &iter, 0, &bflow));

• /* get the usb device which has a backing file (flash1.img) */
• ut_asserteq(0, bootflow_scan_next(&iter, &bflow));
• ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow)); ut_asserteq(6, iter.num_devs); ut_asserteq_str("mmc2.bootdev", iter.dev_used[0]->name);

@@ -330,6 +342,10 @@ static int bootdev_test_prio(struct unit_test_state *uts) bootflow_iter_uninit(&iter); ut_assertok(bootflow_scan_first(NULL, NULL, &iter, BOOTFLOWIF_HUNT, &bflow));

• /* get the usb device which has a backing file (flash1.img) */
• ut_asserteq(0, bootflow_scan_next(&iter, &bflow));
• ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow)); ut_asserteq(7, iter.num_devs); ut_asserteq_str("usb_mass_storage.lun0.bootdev",

diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index da55d5cfe69..37884dbd441 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -525,7 +525,7 @@ static int prep_mmc_bootdev(struct unit_test_state *uts, const char *mmc_dev,

order[2] = mmc_dev;

• /* Enable the mmc4 node since we need a second bootflow */

• /* Enable the requested mmc node since we need a second bootflow */ root = oftree_root(oftree_default()); node = ofnode_find_subnode(root, mmc_dev); ut_assert(ofnode_valid(node));

diff --git a/test/py/tests/bootstd/flash1.img.xz b/test/py/tests/bootstd/flash1.img.xz new file mode 100644 index 0000000000000000000000000000000000000000..9148b91a20c1f0acb54449b124d1408961e92507 GIT binary patch literal 5016 zcmeI0cTf{r8pRVz9+4n5AV?_EgQ1BO;n5K(kx-;pUwkwL0tBUqlz==yF+ikBDAExL zQWT{~2Sq?y06~gW5eP+U$nMPkwKMCEzInSlv*Z7F=Fa`i`OZ1tr78#8*Z}|x3nSGR z=>Wn&ZU6ufAUmH=qlqxW9033y>XG5m*pL}cy$rvQVYbMeTPGAi$9qqfMnd47Jp+<S zIxLcX-gdy?;z*~}QgHUO>b8@aTLW9^ZgBe#gV-u~nri0|VMr5lA6KhSV}ds3x_d!R zK70s*W-GN^=;}ze2DKy`8Bp8a3s-y^i!J>$OdpXOt?{Uc-H+4p?j<BRmf`kr@RkOb z#nr%a+fVcMj7<BT<4=wb&7}i+1?+`%_9rcV-IGFt)xzJvuYasj+h}W$aV}-mF#cWV z0q02?Tk;;}mhB#MFO491k+tL~bm}lePR6P^F?uU1qWM~zn8n@>UP+#@pmNRUwJ*{D zGJt0<ec}SEe@BuyWsjlSn3!f77C4m?PYr2;FS!1eCM{1>cjopJaH2B8t_BV*T|(V? zQE{)kaF4zAwh?({NItl!v80%xP5j+-km~m0FjHOI&3eSrr0=y}ILxn<Dj+V+FtA;? zkfwXZQuFzB-RUPwyo#9fJZ3vFLqjRnRXx{Cjj$``T7)LJeniuGCW|XG2Di@RTK-7v z%@LmQG%VY9Q*iQk{IX5!h|sU6x5>&A^gJ={@<k08%TNsA;LO8|NDKjWvI9|*L4$MB z+7n%gV=Mx1tAg1*_I$;8%e5KZC}1lz<@sfc9Rl028JFGGi3IeoQhBA1Rw!e4KbgZ> zcWW%yKY}Nl<I~z}t_$|PJU0<-g!)ivhK&n#A9YhnK-3N*Q`%=WA+6TycBy`{rYBep zuUe97ouG<2ucIu+XU;2LR8g8yklNz3;<k<<Au8XakMkm_=cM5gt`}-7tJ^bCnR7tn zBe3b$z#jhsLv05PN&NHU+npZ4f?Y<fTmv;LxJ0w1oHl!Nb^<(FgZXuRy)xXYp~*LD z3L*PG^Lc*574Eh7oi!F-PcV5j<eaGzxJ8`ao#8ImP6c^lrB7Shz+u!o#+lun*$NN4 ze+u>1tu^^*#UCxf;bJ&NSo@TX5>5e+pT?chViS8Qddj*v2(zyvvS5|tWhS65&b1s# ztq%uj(ECDcNKU@WQlPgiVOeS{T^}+v^G!^rLMD_i<okVaaHNxvN%DKEC_OJjtJ`fy zKFE0k6@0_k?{_0d-|8&uGq1;2=W4>2e;x1$aOEia;LV&Z930_1Thek8ad0!1k*%M3 z_Q0A`8vzaWYYX7vQ=^y;COH>9zmbcNam0;YlMB3IVMi><k<II&(g=3_UBMw^HCFC+ z@)H?uigXDBXA74rM>ftHk8xY%(%}{iymL<`3XMZW+f8a2?SETgT0;aAgL=a#3!1KX z^cLebMqp!W-R{zh?QA#i^JT<kvfv0(8ry`yg<(qzh1_zt_{3BP*1~C5OmS7q)YTXx zPX4FbYU!lv4)?BM&@_$q$I|V>ju!<g<X?nd#FT-)ZWn*qLX7tafjq@`!P?D{S5IGJ zuvsiPZc%MJ!b8(+vK$@C`6S1;#b%n~3cgfSaXr-V3RSy1Czll1NJTo9Lb$}O7Z-C- zx?3&7@!8q&vQI%h9&~esojd+AF!$PiZ2Gn^%Tq%h_1PQ80p&ToqgoX4NUYYmH|n;P ztuS}UI)h4dAc_9_X4%0uX8Ct>o5tn8#754Z(}^I`q<{*5Rey^jYGXyHJRh;{p1ikj zc9Dqls^pvK0YX|_7#zPTf!*b82Bk?#z3fQBiu4q*qMiE6{UF>)MUAS7qBGw*k4&SI zpAJ7@{r3j-*FNfpyz$Rntz!RCiAZAuN`Ei6|5Fa%9ZzPgnpH&FE1=r9f!*ynU>LRk zs^i2fv8m7Ts$<41M5$$$O!aU5TsIe9FH*b*iXYp#Gs1FO^Y+cMC@x{|f&W>e{yEir zH?4mW<^(Pg@=wM6Kq2~v;(m_kcZ=>Pupg}4KY{%zU`U3zZ->A?w~ha5Y7#LChpCq# zjLs}(gwV(qHKH0PIq==G%UE&4)cV#zQp!hu3P)N#5~J*rP#)Mrnf2FEJB=>p2`NB0 zxrbCGW-cOk>(@E=<>XoI^r+-qsU_Hbj=2C;jaT42`)4Nl?$<AeJrorX|6Ggdh?op? zoEJg?^i<Tb`^gBPlWi0Ad}h<6eb;Olmg82pe%EB`<o%Hrcp--3u;QdeNlf1~)~}X+ z2_`kvu^$BMA*_~WOoBRm2Y2qwq_--k{wDL%<F=&7bN=H4xn;G>A9Ao502gVwPUdyH zk|c8N(S#6;Vbf5B1-#i5rvvuH#*!j4W%J!DHrzLL7Y}2FiA;&kE{gOSUX)eQM58e% zxzCf&=qNe#7eNq+P!z(uHeIWDRQiY=vVq^N5#m=JF!q>=l3~T}x`dQCQkTnB7Jyp9 z9<Ke9SBS3!ukGU#4CI-@!yY10$Hu`~TkU;%B9<w_?}51x8UCnvkWast$>!$S*si%Y z#K@3yE)2HKtuQf<(p%-MP;{u#ln=wpQ1YA#gF=REU4Ysw)eE4nJR61Dj5RfBZ~dM* zSMp96hovw!v!`3u@q#5n+3D*c)52Q{z19NhVU|dnO=-{cW*eIX_2Q97jaK23vWBG7 z@m-4+V3|8nS|8n5(7fuqxU~m1hiT=)sP4+=FIXUT1|kn%EzpMCxRuaG>(_h94n9E@ z^-0~gvKr&93iu$m=JxNvH`Z>5eVq0gWqPV|6Dd^9zrpc%XLqFfRKYs#OhX;3>Jm;b zh)!B^-RF<)Tr}SWQrPsyLGNqxRRqOIG#PiTgwEoM7^!y@5qBOR-Cf%U)jHUm7=x1n zabX0NS!nxd?gq)JryA$B@Ibw^qlg)E=DN@e$6aC^s8+O<N*T+sZtE3VJS6CHR;Hfb zj6!OYV>_{x@AiR~<_W*A<j@8rZy})!cUSg4dNp5pXk>_j#gJdl-^e|Hn)_#5WHWmA z*>1210pl<x|1SJEm1hL6rCToRBa2%(;ktp6={I?DPoB?_h#?1Y%pMU_Pnn5X#zFHB zkD8>k_~Sa)Oa|ASTX<>*J$=fQh1r%Sk4yV+%E8c2*ol{zZAGEORz}_=pJYbILT?~9 zx@Ww_ZY!Phs9}DZ;|(-K7u0?|<x*!^Q((Pp0U@L0j5%*kym>tedsc3u1!ajplM14D z?)FvVYF1EAD}VwEmh!>y)l!<B_<wT8$jHNU{OnXe*r~#qv;d5N2DZJiLI6N8ou4Vd g!6FL)+!7BD4?iLFVyT<d=5|Q;_y0ElgRR})0MlW=WB>pF

literal 0 HcmV?d00001

This reminds me of the xy hack. We should not add binary files. Please, create it on the fly.

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

Please, do not use sudo in tests.

Best regards

Heinrich

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

On Mon, Sep 16, 2024 at 05:42:42PM +0200, Simon Glass wrote:

Hi Heinrich,

On Thu, 12 Sept 2024 at 09:09, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 02.09.24 03:18, 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.

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

(no changes since v1)

arch/sandbox/dts/test.dts | 2 +- test/boot/bootdev.c | 18 +++++++++- test/boot/bootflow.c | 2 +- test/py/tests/bootstd/flash1.img.xz | Bin 0 -> 5016 bytes test/py/tests/test_ut.py | 52 ++++++++++++++++++++++++---- 5 files changed, 65 insertions(+), 9 deletions(-) create mode 100644 test/py/tests/bootstd/flash1.img.xz

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index 5fb5eac862e..a99de6e62ce 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -1507,7 +1507,7 @@ flash-stick@1 { reg = <1>; compatible = "sandbox,usb-flash";

•                                 sandbox,filepath = "testflash1.bin";
  

•                                 sandbox,filepath = "flash1.img";
                          };
  
                          flash-stick@2 {
  

diff --git a/test/boot/bootdev.c b/test/boot/bootdev.c index c635d06ec25..269bcd693a6 100644 --- a/test/boot/bootdev.c +++ b/test/boot/bootdev.c @@ -212,6 +212,10 @@ static int bootdev_test_order(struct unit_test_state *uts) /* Use the environment variable to override it */ ut_assertok(env_set("boot_targets", "mmc1 mmc2 usb")); ut_assertok(bootflow_scan_first(NULL, NULL, &iter, 0, &bflow));

• /* get the usb device which has a backing file (flash1.img) */
  

• ut_asserteq(0, bootflow_scan_next(&iter, &bflow));
  

• ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
  ut_asserteq(5, iter.num_devs);
  ut_asserteq_str("mmc1.bootdev", iter.dev_used[0]->name);
  

@@ -251,7 +255,11 @@ static int bootdev_test_order(struct unit_test_state *uts) ut_assertok(bootflow_scan_first(NULL, NULL, &iter, 0, &bflow)); ut_asserteq(2, iter.num_devs);

• /* Now scan past mmc1 and make sure that the 3 USB devices show up */
  

• /*
  

•  * Now scan past mmc1 and make sure that the 3 USB devices show up. The
  

•  * first one has a backing file so returns success
  

•  */
  

• ut_asserteq(0, bootflow_scan_next(&iter, &bflow));
  ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
  ut_asserteq(6, iter.num_devs);
  ut_asserteq_str("mmc2.bootdev", iter.dev_used[0]->name);
  

@@ -313,6 +321,10 @@ static int bootdev_test_prio(struct unit_test_state *uts)

  /* 3 MMC and 3 USB bootdevs: MMC should come before USB */
  ut_assertok(bootflow_scan_first(NULL, NULL, &iter, 0, &bflow));

• /* get the usb device which has a backing file (flash1.img) */
  

• ut_asserteq(0, bootflow_scan_next(&iter, &bflow));
  

• ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
  ut_asserteq(6, iter.num_devs);
  ut_asserteq_str("mmc2.bootdev", iter.dev_used[0]->name);
  

@@ -330,6 +342,10 @@ static int bootdev_test_prio(struct unit_test_state *uts) bootflow_iter_uninit(&iter); ut_assertok(bootflow_scan_first(NULL, NULL, &iter, BOOTFLOWIF_HUNT, &bflow));

• /* get the usb device which has a backing file (flash1.img) */
  

• ut_asserteq(0, bootflow_scan_next(&iter, &bflow));
  

• ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
  ut_asserteq(7, iter.num_devs);
  ut_asserteq_str("usb_mass_storage.lun0.bootdev",
  

diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index da55d5cfe69..37884dbd441 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -525,7 +525,7 @@ static int prep_mmc_bootdev(struct unit_test_state *uts, const char *mmc_dev,

  order[2] = mmc_dev;

• /* Enable the mmc4 node since we need a second bootflow */
  

• /* Enable the requested mmc node since we need a second bootflow */
  root = oftree_root(oftree_default());
  node = ofnode_find_subnode(root, mmc_dev);
  ut_assert(ofnode_valid(node));
  

diff --git a/test/py/tests/bootstd/flash1.img.xz b/test/py/tests/bootstd/flash1.img.xz new file mode 100644 index 0000000000000000000000000000000000000000..9148b91a20c1f0acb54449b124d1408961e92507 GIT binary patch literal 5016 zcmeI0cTf{r8pRVz9+4n5AV?_EgQ1BO;n5K(kx-;pUwkwL0tBUqlz==yF+ikBDAExL zQWT{~2Sq?y06~gW5eP+U$nMPkwKMCEzInSlv*Z7F=Fa`i`OZ1tr78#8*Z}|x3nSGR z=>Wn&ZU6ufAUmH=qlqxW9033y>XG5m*pL}cy$rvQVYbMeTPGAi$9qqfMnd47Jp+<S zIxLcX-gdy?;z*~}QgHUO>b8@aTLW9^ZgBe#gV-u~nri0|VMr5lA6KhSV}ds3x_d!R zK70s*W-GN^=;}ze2DKy`8Bp8a3s-y^i!J>$OdpXOt?{Uc-H+4p?j<BRmf`kr@RkOb z#nr%a+fVcMj7<BT<4=wb&7}i+1?+`%_9rcV-IGFt)xzJvuYasj+h}W$aV}-mF#cWV z0q02?Tk;;}mhB#MFO491k+tL~bm}lePR6P^F?uU1qWM~zn8n@>UP+#@pmNRUwJ*{D zGJt0<ec}SEe@BuyWsjlSn3!f77C4m?PYr2;FS!1eCM{1>cjopJaH2B8t_BV*T|(V? zQE{)kaF4zAwh?({NItl!v80%xP5j+-km~m0FjHOI&3eSrr0=y}ILxn<Dj+V+FtA;? zkfwXZQuFzB-RUPwyo#9fJZ3vFLqjRnRXx{Cjj$``T7)LJeniuGCW|XG2Di@RTK-7v z%@LmQG%VY9Q*iQk{IX5!h|sU6x5>&A^gJ={@<k08%TNsA;LO8|NDKjWvI9|*L4$MB z+7n%gV=Mx1tAg1*_I$;8%e5KZC}1lz<@sfc9Rl028JFGGi3IeoQhBA1Rw!e4KbgZ> zcWW%yKY}Nl<I~z}t_$|PJU0<-g!)ivhK&n#A9YhnK-3N*Q`%=WA+6TycBy`{rYBep zuUe97ouG<2ucIu+XU;2LR8g8yklNz3;<k<<Au8XakMkm_=cM5gt`}-7tJ^bCnR7tn zBe3b$z#jhsLv05PN&NHU+npZ4f?Y<fTmv;LxJ0w1oHl!Nb^<(FgZXuRy)xXYp~*LD z3L*PG^Lc*574Eh7oi!F-PcV5j<eaGzxJ8`ao#8ImP6c^lrB7Shz+u!o#+lun*$NN4 ze+u>1tu^^*#UCxf;bJ&NSo@TX5>5e+pT?chViS8Qddj*v2(zyvvS5|tWhS65&b1s# ztq%uj(ECDcNKU@WQlPgiVOeS{T^}+v^G!^rLMD_i<okVaaHNxvN%DKEC_OJjtJ`fy zKFE0k6@0_k?{_0d-|8&uGq1;2=W4>2e;x1$aOEia;LV&Z930_1Thek8ad0!1k*%M3 z_Q0A`8vzaWYYX7vQ=^y;COH>9zmbcNam0;YlMB3IVMi><k<II&(g=3_UBMw^HCFC+ z@)H?uigXDBXA74rM>ftHk8xY%(%}{iymL<`3XMZW+f8a2?SETgT0;aAgL=a#3!1KX z^cLebMqp!W-R{zh?QA#i^JT<kvfv0(8ry`yg<(qzh1_zt_{3BP*1~C5OmS7q)YTXx zPX4FbYU!lv4)?BM&@_$q$I|V>ju!<g<X?nd#FT-)ZWn*qLX7tafjq@`!P?D{S5IGJ zuvsiPZc%MJ!b8(+vK$@C`6S1;#b%n~3cgfSaXr-V3RSy1Czll1NJTo9Lb$}O7Z-C- zx?3&7@!8q&vQI%h9&~esojd+AF!$PiZ2Gn^%Tq%h_1PQ80p&ToqgoX4NUYYmH|n;P ztuS}UI)h4dAc_9_X4%0uX8Ct>o5tn8#754Z(}^I`q<{*5Rey^jYGXyHJRh;{p1ikj zc9Dqls^pvK0YX|_7#zPTf!*b82Bk?#z3fQBiu4q*qMiE6{UF>)MUAS7qBGw*k4&SI zpAJ7@{r3j-*FNfpyz$Rntz!RCiAZAuN`Ei6|5Fa%9ZzPgnpH&FE1=r9f!*ynU>LRk zs^i2fv8m7Ts$<41M5$$$O!aU5TsIe9FH*b*iXYp#Gs1FO^Y+cMC@x{|f&W>e{yEir zH?4mW<^(Pg@=wM6Kq2~v;(m_kcZ=>Pupg}4KY{%zU`U3zZ->A?w~ha5Y7#LChpCq# zjLs}(gwV(qHKH0PIq==G%UE&4)cV#zQp!hu3P)N#5~J*rP#)Mrnf2FEJB=>p2`NB0 zxrbCGW-cOk>(@E=<>XoI^r+-qsU_Hbj=2C;jaT42`)4Nl?$<AeJrorX|6Ggdh?op? zoEJg?^i<Tb`^gBPlWi0Ad}h<6eb;Olmg82pe%EB`<o%Hrcp--3u;QdeNlf1~)~}X+ z2_`kvu^$BMA*_~WOoBRm2Y2qwq_--k{wDL%<F=&7bN=H4xn;G>A9Ao502gVwPUdyH zk|c8N(S#6;Vbf5B1-#i5rvvuH#*!j4W%J!DHrzLL7Y}2FiA;&kE{gOSUX)eQM58e% zxzCf&=qNe#7eNq+P!z(uHeIWDRQiY=vVq^N5#m=JF!q>=l3~T}x`dQCQkTnB7Jyp9 z9<Ke9SBS3!ukGU#4CI-@!yY10$Hu`~TkU;%B9<w_?}51x8UCnvkWast$>!$S*si%Y z#K@3yE)2HKtuQf<(p%-MP;{u#ln=wpQ1YA#gF=REU4Ysw)eE4nJR61Dj5RfBZ~dM* zSMp96hovw!v!`3u@q#5n+3D*c)52Q{z19NhVU|dnO=-{cW*eIX_2Q97jaK23vWBG7 z@m-4+V3|8nS|8n5(7fuqxU~m1hiT=)sP4+=FIXUT1|kn%EzpMCxRuaG>(_h94n9E@ z^-0~gvKr&93iu$m=JxNvH`Z>5eVq0gWqPV|6Dd^9zrpc%XLqFfRKYs#OhX;3>Jm;b zh)!B^-RF<)Tr}SWQrPsyLGNqxRRqOIG#PiTgwEoM7^!y@5qBOR-Cf%U)jHUm7=x1n zabX0NS!nxd?gq)JryA$B@Ibw^qlg)E=DN@e$6aC^s8+O<N*T+sZtE3VJS6CHR;Hfb zj6!OYV>_{x@AiR~<_W*A<j@8rZy})!cUSg4dNp5pXk>_j#gJdl-^e|Hn)_#5WHWmA z*>1210pl<x|1SJEm1hL6rCToRBa2%(;ktp6={I?DPoB?_h#?1Y%pMU_Pnn5X#zFHB zkD8>k_~Sa)Oa|ASTX<>*J$=fQh1r%Sk4yV+%E8c2*ol{zZAGEORz}_=pJYbILT?~9 zx@Ww_ZY!Phs9}DZ;|(-K7u0?|<x*!^Q((Pp0U@L0j5%*kym>tedsc3u1!ajplM14D z?)FvVYF1EAD}VwEmh!>y)l!<B_<wT8$jHNU{OnXe*r~#qv;d5N2DZJiLI6N8ou4Vd g!6FL)+!7BD4?iLFVyT<d=5|Q;_y0ElgRR})0MlW=WB>pF

literal 0 HcmV?d00001

This reminds me of the xy hack. We should not add binary files. Please, create it on the fly.

All the other tests work the same way.

...

...

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

Please, do not use sudo in tests.

All the other test setup uses sudo, as do the fs-tests

We're actively removing the use of sudo in the fs-tests, as noted in previous iterations of this series (I believe it was this series) and asked that you rebase this part at least on top of that series or wait for it to be merged.

On Tue, Sep 17, 2024 at 05:52:09AM +0200, Simon Glass wrote:

Hi Tom,

On Mon, 16 Sept 2024 at 18:34, Tom Rini trini@konsulko.com wrote:

...

On Mon, Sep 16, 2024 at 05:42:42PM +0200, Simon Glass wrote:

...

Hi Heinrich,

On Thu, 12 Sept 2024 at 09:09, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 02.09.24 03:18, 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.

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

(no changes since v1)

arch/sandbox/dts/test.dts | 2 +- test/boot/bootdev.c | 18 +++++++++- test/boot/bootflow.c | 2 +- test/py/tests/bootstd/flash1.img.xz | Bin 0 -> 5016 bytes test/py/tests/test_ut.py | 52 ++++++++++++++++++++++++---- 5 files changed, 65 insertions(+), 9 deletions(-) create mode 100644 test/py/tests/bootstd/flash1.img.xz

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index 5fb5eac862e..a99de6e62ce 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -1507,7 +1507,7 @@ flash-stick@1 { reg = <1>; compatible = "sandbox,usb-flash";

•                                 sandbox,filepath = "testflash1.bin";
  

•                                 sandbox,filepath = "flash1.img";
                          };
  
                          flash-stick@2 {
  

diff --git a/test/boot/bootdev.c b/test/boot/bootdev.c index c635d06ec25..269bcd693a6 100644 --- a/test/boot/bootdev.c +++ b/test/boot/bootdev.c @@ -212,6 +212,10 @@ static int bootdev_test_order(struct unit_test_state *uts) /* Use the environment variable to override it */ ut_assertok(env_set("boot_targets", "mmc1 mmc2 usb")); ut_assertok(bootflow_scan_first(NULL, NULL, &iter, 0, &bflow));

• /* get the usb device which has a backing file (flash1.img) */
  

• ut_asserteq(0, bootflow_scan_next(&iter, &bflow));
  

• ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
  ut_asserteq(5, iter.num_devs);
  ut_asserteq_str("mmc1.bootdev", iter.dev_used[0]->name);
  

@@ -251,7 +255,11 @@ static int bootdev_test_order(struct unit_test_state *uts) ut_assertok(bootflow_scan_first(NULL, NULL, &iter, 0, &bflow)); ut_asserteq(2, iter.num_devs);

• /* Now scan past mmc1 and make sure that the 3 USB devices show up */
  

• /*
  

•  * Now scan past mmc1 and make sure that the 3 USB devices show up. The
  

•  * first one has a backing file so returns success
  

•  */
  

• ut_asserteq(0, bootflow_scan_next(&iter, &bflow));
  ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
  ut_asserteq(6, iter.num_devs);
  ut_asserteq_str("mmc2.bootdev", iter.dev_used[0]->name);
  

@@ -313,6 +321,10 @@ static int bootdev_test_prio(struct unit_test_state *uts)

  /* 3 MMC and 3 USB bootdevs: MMC should come before USB */
  ut_assertok(bootflow_scan_first(NULL, NULL, &iter, 0, &bflow));

• /* get the usb device which has a backing file (flash1.img) */
  

• ut_asserteq(0, bootflow_scan_next(&iter, &bflow));
  

• ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
  ut_asserteq(6, iter.num_devs);
  ut_asserteq_str("mmc2.bootdev", iter.dev_used[0]->name);
  

@@ -330,6 +342,10 @@ static int bootdev_test_prio(struct unit_test_state *uts) bootflow_iter_uninit(&iter); ut_assertok(bootflow_scan_first(NULL, NULL, &iter, BOOTFLOWIF_HUNT, &bflow));

• /* get the usb device which has a backing file (flash1.img) */
  

• ut_asserteq(0, bootflow_scan_next(&iter, &bflow));
  

• ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow));
  ut_asserteq(7, iter.num_devs);
  ut_asserteq_str("usb_mass_storage.lun0.bootdev",
  

diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index da55d5cfe69..37884dbd441 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -525,7 +525,7 @@ static int prep_mmc_bootdev(struct unit_test_state *uts, const char *mmc_dev,

  order[2] = mmc_dev;

• /* Enable the mmc4 node since we need a second bootflow */
  

• /* Enable the requested mmc node since we need a second bootflow */
  root = oftree_root(oftree_default());
  node = ofnode_find_subnode(root, mmc_dev);
  ut_assert(ofnode_valid(node));
  

diff --git a/test/py/tests/bootstd/flash1.img.xz b/test/py/tests/bootstd/flash1.img.xz new file mode 100644 index 0000000000000000000000000000000000000000..9148b91a20c1f0acb54449b124d1408961e92507 GIT binary patch literal 5016 zcmeI0cTf{r8pRVz9+4n5AV?_EgQ1BO;n5K(kx-;pUwkwL0tBUqlz==yF+ikBDAExL zQWT{~2Sq?y06~gW5eP+U$nMPkwKMCEzInSlv*Z7F=Fa`i`OZ1tr78#8*Z}|x3nSGR z=>Wn&ZU6ufAUmH=qlqxW9033y>XG5m*pL}cy$rvQVYbMeTPGAi$9qqfMnd47Jp+<S zIxLcX-gdy?;z*~}QgHUO>b8@aTLW9^ZgBe#gV-u~nri0|VMr5lA6KhSV}ds3x_d!R zK70s*W-GN^=;}ze2DKy`8Bp8a3s-y^i!J>$OdpXOt?{Uc-H+4p?j<BRmf`kr@RkOb z#nr%a+fVcMj7<BT<4=wb&7}i+1?+`%_9rcV-IGFt)xzJvuYasj+h}W$aV}-mF#cWV z0q02?Tk;;}mhB#MFO491k+tL~bm}lePR6P^F?uU1qWM~zn8n@>UP+#@pmNRUwJ*{D zGJt0<ec}SEe@BuyWsjlSn3!f77C4m?PYr2;FS!1eCM{1>cjopJaH2B8t_BV*T|(V? zQE{)kaF4zAwh?({NItl!v80%xP5j+-km~m0FjHOI&3eSrr0=y}ILxn<Dj+V+FtA;? zkfwXZQuFzB-RUPwyo#9fJZ3vFLqjRnRXx{Cjj$``T7)LJeniuGCW|XG2Di@RTK-7v z%@LmQG%VY9Q*iQk{IX5!h|sU6x5>&A^gJ={@<k08%TNsA;LO8|NDKjWvI9|*L4$MB z+7n%gV=Mx1tAg1*_I$;8%e5KZC}1lz<@sfc9Rl028JFGGi3IeoQhBA1Rw!e4KbgZ> zcWW%yKY}Nl<I~z}t_$|PJU0<-g!)ivhK&n#A9YhnK-3N*Q`%=WA+6TycBy`{rYBep zuUe97ouG<2ucIu+XU;2LR8g8yklNz3;<k<<Au8XakMkm_=cM5gt`}-7tJ^bCnR7tn zBe3b$z#jhsLv05PN&NHU+npZ4f?Y<fTmv;LxJ0w1oHl!Nb^<(FgZXuRy)xXYp~*LD z3L*PG^Lc*574Eh7oi!F-PcV5j<eaGzxJ8`ao#8ImP6c^lrB7Shz+u!o#+lun*$NN4 ze+u>1tu^^*#UCxf;bJ&NSo@TX5>5e+pT?chViS8Qddj*v2(zyvvS5|tWhS65&b1s# ztq%uj(ECDcNKU@WQlPgiVOeS{T^}+v^G!^rLMD_i<okVaaHNxvN%DKEC_OJjtJ`fy zKFE0k6@0_k?{_0d-|8&uGq1;2=W4>2e;x1$aOEia;LV&Z930_1Thek8ad0!1k*%M3 z_Q0A`8vzaWYYX7vQ=^y;COH>9zmbcNam0;YlMB3IVMi><k<II&(g=3_UBMw^HCFC+ z@)H?uigXDBXA74rM>ftHk8xY%(%}{iymL<`3XMZW+f8a2?SETgT0;aAgL=a#3!1KX z^cLebMqp!W-R{zh?QA#i^JT<kvfv0(8ry`yg<(qzh1_zt_{3BP*1~C5OmS7q)YTXx zPX4FbYU!lv4)?BM&@_$q$I|V>ju!<g<X?nd#FT-)ZWn*qLX7tafjq@`!P?D{S5IGJ zuvsiPZc%MJ!b8(+vK$@C`6S1;#b%n~3cgfSaXr-V3RSy1Czll1NJTo9Lb$}O7Z-C- zx?3&7@!8q&vQI%h9&~esojd+AF!$PiZ2Gn^%Tq%h_1PQ80p&ToqgoX4NUYYmH|n;P ztuS}UI)h4dAc_9_X4%0uX8Ct>o5tn8#754Z(}^I`q<{*5Rey^jYGXyHJRh;{p1ikj zc9Dqls^pvK0YX|_7#zPTf!*b82Bk?#z3fQBiu4q*qMiE6{UF>)MUAS7qBGw*k4&SI zpAJ7@{r3j-*FNfpyz$Rntz!RCiAZAuN`Ei6|5Fa%9ZzPgnpH&FE1=r9f!*ynU>LRk zs^i2fv8m7Ts$<41M5$$$O!aU5TsIe9FH*b*iXYp#Gs1FO^Y+cMC@x{|f&W>e{yEir zH?4mW<^(Pg@=wM6Kq2~v;(m_kcZ=>Pupg}4KY{%zU`U3zZ->A?w~ha5Y7#LChpCq# zjLs}(gwV(qHKH0PIq==G%UE&4)cV#zQp!hu3P)N#5~J*rP#)Mrnf2FEJB=>p2`NB0 zxrbCGW-cOk>(@E=<>XoI^r+-qsU_Hbj=2C;jaT42`)4Nl?$<AeJrorX|6Ggdh?op? zoEJg?^i<Tb`^gBPlWi0Ad}h<6eb;Olmg82pe%EB`<o%Hrcp--3u;QdeNlf1~)~}X+ z2_`kvu^$BMA*_~WOoBRm2Y2qwq_--k{wDL%<F=&7bN=H4xn;G>A9Ao502gVwPUdyH zk|c8N(S#6;Vbf5B1-#i5rvvuH#*!j4W%J!DHrzLL7Y}2FiA;&kE{gOSUX)eQM58e% zxzCf&=qNe#7eNq+P!z(uHeIWDRQiY=vVq^N5#m=JF!q>=l3~T}x`dQCQkTnB7Jyp9 z9<Ke9SBS3!ukGU#4CI-@!yY10$Hu`~TkU;%B9<w_?}51x8UCnvkWast$>!$S*si%Y z#K@3yE)2HKtuQf<(p%-MP;{u#ln=wpQ1YA#gF=REU4Ysw)eE4nJR61Dj5RfBZ~dM* zSMp96hovw!v!`3u@q#5n+3D*c)52Q{z19NhVU|dnO=-{cW*eIX_2Q97jaK23vWBG7 z@m-4+V3|8nS|8n5(7fuqxU~m1hiT=)sP4+=FIXUT1|kn%EzpMCxRuaG>(_h94n9E@ z^-0~gvKr&93iu$m=JxNvH`Z>5eVq0gWqPV|6Dd^9zrpc%XLqFfRKYs#OhX;3>Jm;b zh)!B^-RF<)Tr}SWQrPsyLGNqxRRqOIG#PiTgwEoM7^!y@5qBOR-Cf%U)jHUm7=x1n zabX0NS!nxd?gq)JryA$B@Ibw^qlg)E=DN@e$6aC^s8+O<N*T+sZtE3VJS6CHR;Hfb zj6!OYV>_{x@AiR~<_W*A<j@8rZy})!cUSg4dNp5pXk>_j#gJdl-^e|Hn)_#5WHWmA z*>1210pl<x|1SJEm1hL6rCToRBa2%(;ktp6={I?DPoB?_h#?1Y%pMU_Pnn5X#zFHB zkD8>k_~Sa)Oa|ASTX<>*J$=fQh1r%Sk4yV+%E8c2*ol{zZAGEORz}_=pJYbILT?~9 zx@Ww_ZY!Phs9}DZ;|(-K7u0?|<x*!^Q((Pp0U@L0j5%*kym>tedsc3u1!ajplM14D z?)FvVYF1EAD}VwEmh!>y)l!<B_<wT8$jHNU{OnXe*r~#qv;d5N2DZJiLI6N8ou4Vd g!6FL)+!7BD4?iLFVyT<d=5|Q;_y0ElgRR})0MlW=WB>pF

literal 0 HcmV?d00001

This reminds me of the xy hack. We should not add binary files. Please, create it on the fly.

All the other tests work the same way.

...

...

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

Please, do not use sudo in tests.

All the other test setup uses sudo, as do the fs-tests

We're actively removing the use of sudo in the fs-tests, as noted in previous iterations of this series (I believe it was this series) and asked that you rebase this part at least on top of that series or wait for it to be merged.

Well I don't believe there is actually an updated patch for this. So I don't believe it can be merged?

If I missed it, please can you send the patchwork link?

If and when it is merged, it is a trivial patch to update this to conform, as I noted before when this was raised.

I've followed up, today. But on the other hand the problems were: - Minor pylint problems - Need one other change to go in first due to how terribly the try/catch stuff has left things.

And would prefer (a) waiting or (b) you pick it up and finish the last 5% and mark it as a pre-req to (c) we add yet another sudo based test that isn't run as often as the others. Sorry.

On Thu, Sep 19, 2024 at 04:13:01PM +0200, Simon Glass wrote:

Hi Tom,

On Wed, 18 Sept 2024 at 00:19, Tom Rini trini@konsulko.com wrote:

...

On Tue, Sep 17, 2024 at 05:52:09AM +0200, Simon Glass wrote:

...

Hi Tom,

On Mon, 16 Sept 2024 at 18:34, Tom Rini trini@konsulko.com wrote:

...

On Mon, Sep 16, 2024 at 05:42:42PM +0200, Simon Glass wrote:

...

Hi Heinrich,

On Thu, 12 Sept 2024 at 09:09, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 02.09.24 03:18, 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. > > Signed-off-by: Simon Glass sjg@chromium.org > --- > > (no changes since v1) > > arch/sandbox/dts/test.dts | 2 +- > test/boot/bootdev.c | 18 +++++++++- > test/boot/bootflow.c | 2 +- > test/py/tests/bootstd/flash1.img.xz | Bin 0 -> 5016 bytes > test/py/tests/test_ut.py | 52 ++++++++++++++++++++++++---- > 5 files changed, 65 insertions(+), 9 deletions(-) > create mode 100644 test/py/tests/bootstd/flash1.img.xz > > diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts > index 5fb5eac862e..a99de6e62ce 100644 > --- a/arch/sandbox/dts/test.dts > +++ b/arch/sandbox/dts/test.dts > @@ -1507,7 +1507,7 @@ > flash-stick@1 { > reg = <1>; > compatible = "sandbox,usb-flash"; > - sandbox,filepath = "testflash1.bin"; > + sandbox,filepath = "flash1.img"; > }; > > flash-stick@2 { > diff --git a/test/boot/bootdev.c b/test/boot/bootdev.c > index c635d06ec25..269bcd693a6 100644 > --- a/test/boot/bootdev.c > +++ b/test/boot/bootdev.c > @@ -212,6 +212,10 @@ static int bootdev_test_order(struct unit_test_state *uts) > /* Use the environment variable to override it */ > ut_assertok(env_set("boot_targets", "mmc1 mmc2 usb")); > ut_assertok(bootflow_scan_first(NULL, NULL, &iter, 0, &bflow)); > + > + /* get the usb device which has a backing file (flash1.img) */ > + ut_asserteq(0, bootflow_scan_next(&iter, &bflow)); > + > ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow)); > ut_asserteq(5, iter.num_devs); > ut_asserteq_str("mmc1.bootdev", iter.dev_used[0]->name); > @@ -251,7 +255,11 @@ static int bootdev_test_order(struct unit_test_state *uts) > ut_assertok(bootflow_scan_first(NULL, NULL, &iter, 0, &bflow)); > ut_asserteq(2, iter.num_devs); > > - /* Now scan past mmc1 and make sure that the 3 USB devices show up */ > + /* > + * Now scan past mmc1 and make sure that the 3 USB devices show up. The > + * first one has a backing file so returns success > + */ > + ut_asserteq(0, bootflow_scan_next(&iter, &bflow)); > ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow)); > ut_asserteq(6, iter.num_devs); > ut_asserteq_str("mmc2.bootdev", iter.dev_used[0]->name); > @@ -313,6 +321,10 @@ static int bootdev_test_prio(struct unit_test_state *uts) > > /* 3 MMC and 3 USB bootdevs: MMC should come before USB */ > ut_assertok(bootflow_scan_first(NULL, NULL, &iter, 0, &bflow)); > + > + /* get the usb device which has a backing file (flash1.img) */ > + ut_asserteq(0, bootflow_scan_next(&iter, &bflow)); > + > ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow)); > ut_asserteq(6, iter.num_devs); > ut_asserteq_str("mmc2.bootdev", iter.dev_used[0]->name); > @@ -330,6 +342,10 @@ static int bootdev_test_prio(struct unit_test_state *uts) > bootflow_iter_uninit(&iter); > ut_assertok(bootflow_scan_first(NULL, NULL, &iter, BOOTFLOWIF_HUNT, > &bflow)); > + > + /* get the usb device which has a backing file (flash1.img) */ > + ut_asserteq(0, bootflow_scan_next(&iter, &bflow)); > + > ut_asserteq(-ENODEV, bootflow_scan_next(&iter, &bflow)); > ut_asserteq(7, iter.num_devs); > ut_asserteq_str("usb_mass_storage.lun0.bootdev", > diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c > index da55d5cfe69..37884dbd441 100644 > --- a/test/boot/bootflow.c > +++ b/test/boot/bootflow.c > @@ -525,7 +525,7 @@ static int prep_mmc_bootdev(struct unit_test_state *uts, const char *mmc_dev, > > order[2] = mmc_dev; > > - /* Enable the mmc4 node since we need a second bootflow */ > + /* Enable the requested mmc node since we need a second bootflow */ > root = oftree_root(oftree_default()); > node = ofnode_find_subnode(root, mmc_dev); > ut_assert(ofnode_valid(node)); > diff --git a/test/py/tests/bootstd/flash1.img.xz b/test/py/tests/bootstd/flash1.img.xz > new file mode 100644 > index 0000000000000000000000000000000000000000..9148b91a20c1f0acb54449b124d1408961e92507 > GIT binary patch > literal 5016 > zcmeI0cTf{r8pRVz9+4n5AV?_EgQ1BO;n5K(kx-;pUwkwL0tBUqlz==yF+ikBDAExL > zQWT{~2Sq?y06~gW5eP+U$nMPkwKMCEzInSlv*Z7F=Fa`i`OZ1tr78#8*Z}|x3nSGR > z=>Wn&ZU6ufAUmH=qlqxW9033y>XG5m*pL}cy$rvQVYbMeTPGAi$9qqfMnd47Jp+<S > zIxLcX-gdy?;z*~}QgHUO>b8@aTLW9^ZgBe#gV-u~nri0|VMr5lA6KhSV}ds3x_d!R > zK70s*W-GN^=;}ze2DKy`8Bp8a3s-y^i!J>$OdpXOt?{Uc-H+4p?j<BRmf`kr@RkOb > z#nr%a+fVcMj7<BT<4=wb&7}i+1?+`%_9rcV-IGFt)xzJvuYasj+h}W$aV}-mF#cWV > z0q02?Tk;;}mhB#MFO491k+tL~bm}lePR6P^F?uU1qWM~zn8n@>UP+#@pmNRUwJ*{D > zGJt0<ec}SEe@BuyWsjlSn3!f77C4m?PYr2;FS!1eCM{1>cjopJaH2B8t_BV*T|(V? > zQE{)kaF4zAwh?({NItl!v80%xP5j+-km~m0FjHOI&3eSrr0=y}ILxn<Dj+V+FtA;? > zkfwXZQuFzB-RUPwyo#9fJZ3vFLqjRnRXx{Cjj$``T7)LJeniuGCW|XG2Di@RTK-7v > z%@LmQG%VY9Q*iQk{IX5!h|sU6x5>&A^gJ={@<k08%TNsA;LO8|NDKjWvI9|*L4$MB > z+7n%gV=Mx1tAg1*_I$;8%e5KZC}1lz<@sfc9Rl028JFGGi3IeoQhBA1Rw!e4KbgZ> > zcWW%yKY}Nl<I~z}t_$|PJU0<-g!)ivhK&n#A9YhnK-3N*Q`%=WA+6TycBy`{rYBep > zuUe97ouG<2ucIu+XU;2LR8g8yklNz3;<k<<Au8XakMkm_=cM5gt`}-7tJ^bCnR7tn > zBe3b$z#jhsLv05PN&NHU+npZ4f?Y<fTmv;LxJ0w1oHl!Nb^<(FgZXuRy)xXYp~*LD > z3L*PG^Lc*574Eh7oi!F-PcV5j<eaGzxJ8`ao#8ImP6c^lrB7Shz+u!o#+lun*$NN4 > ze+u>1tu^^*#UCxf;bJ&NSo@TX5>5e+pT?chViS8Qddj*v2(zyvvS5|tWhS65&b1s# > ztq%uj(ECDcNKU@WQlPgiVOeS{T^}+v^G!^rLMD_i<okVaaHNxvN%DKEC_OJjtJ`fy > zKFE0k6@0_k?{_0d-|8&uGq1;2=W4>2e;x1$aOEia;LV&Z930_1Thek8ad0!1k*%M3 > z_Q0A`8vzaWYYX7vQ=^y;COH>9zmbcNam0;YlMB3IVMi><k<II&(g=3_UBMw^HCFC+ > z@)H?uigXDBXA74rM>ftHk8xY%(%}{iymL<`3XMZW+f8a2?SETgT0;aAgL=a#3!1KX > z^cLebMqp!W-R{zh?QA#i^JT<kvfv0(8ry`yg<(qzh1_zt_{3BP*1~C5OmS7q)YTXx > zPX4FbYU!lv4)?BM&@_$q$I|V>ju!<g<X?nd#FT-)ZWn*qLX7tafjq@`!P?D{S5IGJ > zuvsiPZc%MJ!b8(+vK$@C`6S1;#b%n~3cgfSaXr-V3RSy1Czll1NJTo9Lb$}O7Z-C- > zx?3&7@!8q&vQI%h9&~esojd+AF!$PiZ2Gn^%Tq%h_1PQ80p&ToqgoX4NUYYmH|n;P > ztuS}UI)h4dAc_9_X4%0uX8Ct>o5tn8#754Z(}^I`q<{*5Rey^jYGXyHJRh;{p1ikj > zc9Dqls^pvK0YX|_7#zPTf!*b82Bk?#z3fQBiu4q*qMiE6{UF>)MUAS7qBGw*k4&SI > zpAJ7@{r3j-*FNfpyz$Rntz!RCiAZAuN`Ei6|5Fa%9ZzPgnpH&FE1=r9f!*ynU>LRk > zs^i2fv8m7Ts$<41M5$$$O!aU5TsIe9FH*b*iXYp#Gs1FO^Y+cMC@x{|f&W>e{yEir > zH?4mW<^(Pg@=wM6Kq2~v;(m_kcZ=>Pupg}4KY{%zU`U3zZ->A?w~ha5Y7#LChpCq# > zjLs}(gwV(qHKH0PIq==G%UE&4)cV#zQp!hu3P)N#5~J*rP#)Mrnf2FEJB=>p2`NB0 > zxrbCGW-cOk>(@E=<>XoI^r+-qsU_Hbj=2C;jaT42`)4Nl?$<AeJrorX|6Ggdh?op? > zoEJg?^i<Tb`^gBPlWi0Ad}h<6eb;Olmg82pe%EB`<o%Hrcp--3u;QdeNlf1~)~}X+ > z2_`kvu^$BMA*_~WOoBRm2Y2qwq_--k{wDL%<F=&7bN=H4xn;G>A9Ao502gVwPUdyH > zk|c8N(S#6;Vbf5B1-#i5rvvuH#*!j4W%J!DHrzLL7Y}2FiA;&kE{gOSUX)eQM58e% > zxzCf&=qNe#7eNq+P!z(uHeIWDRQiY=vVq^N5#m=JF!q>=l3~T}x`dQCQkTnB7Jyp9 > z9<Ke9SBS3!ukGU#4CI-@!yY10$Hu`~TkU;%B9<w_?}51x8UCnvkWast$>!$S*si%Y > z#K@3yE)2HKtuQf<(p%-MP;{u#ln=wpQ1YA#gF=REU4Ysw)eE4nJR61Dj5RfBZ~dM* > zSMp96hovw!v!`3u@q#5n+3D*c)52Q{z19NhVU|dnO=-{cW*eIX_2Q97jaK23vWBG7 > z@m-4+V3|8nS|8n5(7fuqxU~m1hiT=)sP4+=FIXUT1|kn%EzpMCxRuaG>(_h94n9E@ > z^-0~gvKr&93iu$m=JxNvH`Z>5eVq0gWqPV|6Dd^9zrpc%XLqFfRKYs#OhX;3>Jm;b > zh)!B^-RF<)Tr}SWQrPsyLGNqxRRqOIG#PiTgwEoM7^!y@5qBOR-Cf%U)jHUm7=x1n > zabX0NS!nxd?gq)JryA$B@Ibw^qlg)E=DN@e$6aC^s8+O<N*T+sZtE3VJS6CHR;Hfb > zj6!OYV>_{x@AiR~<_W*A<j@8rZy})!cUSg4dNp5pXk>_j#gJdl-^e|Hn)_#5WHWmA > z*>1210pl<x|1SJEm1hL6rCToRBa2%(;ktp6={I?DPoB?_h#?1Y%pMU_Pnn5X#zFHB > zkD8>k_~Sa)Oa|ASTX<>*J$=fQh1r%Sk4yV+%E8c2*ol{zZAGEORz}_=pJYbILT?~9 > zx@Ww_ZY!Phs9}DZ;|(-K7u0?|<x*!^Q((Pp0U@L0j5%*kym>tedsc3u1!ajplM14D > z?)FvVYF1EAD}VwEmh!>y)l!<B_<wT8$jHNU{OnXe*r~#qv;d5N2DZJiLI6N8ou4Vd > g!6FL)+!7BD4?iLFVyT<d=5|Q;_y0ElgRR})0MlW=WB>pF > > literal 0 > HcmV?d00001

This reminds me of the xy hack. We should not add binary files. Please, create it on the fly.

All the other tests work the same way.

...

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

Please, do not use sudo in tests.

All the other test setup uses sudo, as do the fs-tests

We're actively removing the use of sudo in the fs-tests, as noted in previous iterations of this series (I believe it was this series) and asked that you rebase this part at least on top of that series or wait for it to be merged.

Well I don't believe there is actually an updated patch for this. So I don't believe it can be merged?

If I missed it, please can you send the patchwork link?

If and when it is merged, it is a trivial patch to update this to conform, as I noted before when this was raised.

I've followed up, today. But on the other hand the problems were:

• Minor pylint problems
• Need one other change to go in first due to how terribly the try/catch stuff has left things.

And would prefer (a) waiting or (b) you pick it up and finish the last 5% and mark it as a pre-req to (c) we add yet another sudo based test that isn't run as often as the others. Sorry.

How about as a compromise we bring this test in and I send a follow-up series to tidy up the no-sudo patch?

At this point, this series needs to be re-spun either way so, whichever order you want to handle cleaning up the other series is fine, in the end. Thanks.

On 02.09.24 03:18, Simon Glass wrote:

Add a simple test of booting with the EFI bootmeth, which runs the app and checks that it can call 'exit boot-services' (to check that all the device-removal code doesn't break anything) and then exit back to U-Boot.

This uses a disk image containing the testapp, ready for execution by sandbox when needed.

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

---

Changes in v5:

• Rebase on updated efif series
• Deal with sandbox CONFIG_LOGF_FUNC

Changes in v4:

• Add efi_loader tag to some patches
• Split out non-EFI patches into a different series

Changes in v2:

• Add many new patches to resolve all the outstanding test issues

  test/boot/bootflow.c | 64 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)

diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index 37884dbd441..d8da3aac493 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -13,6 +13,7 @@ #include <cli.h> #include <dm.h> #include <efi_default_filename.h> +#include <efi_loader.h> #include <expo.h> #ifdef CONFIG_SANDBOX #include <asm/test.h> @@ -31,6 +32,9 @@ extern U_BOOT_DRIVER(bootmeth_android); extern U_BOOT_DRIVER(bootmeth_cros); extern U_BOOT_DRIVER(bootmeth_2script);

+/* Use this as the vendor for EFI to tell the app to exit boot services */ +static u16 __efi_runtime_data test_vendor[] = u"U-Boot testing";

• static int inject_response(struct unit_test_state *uts) { /*

@@ -1205,3 +1209,63 @@ static int bootflow_android(struct unit_test_state *uts) return 0; } BOOTSTD_TEST(bootflow_android, UTF_CONSOLE);

+/* Test EFI bootmeth */ +static int bootflow_efi(struct unit_test_state *uts) +{

• /* disable ethernet since the hunter will run dhcp */
• test_set_eth_enable(false);
• /* make USB scan without delays */
• test_set_skip_delays(true);
• bootstd_reset_usb();
• /* Avoid outputting ANSI characters which mess with our asserts */
• efi_console_set_ansi(false);
• ut_assertok(bootstd_test_drop_bootdev_order(uts));
• ut_assertok(run_command("bootflow scan", 0));
• ut_assert_skip_to_line(

• "Bus usb@1: scanning bus usb@1 for devices... 5 USB Device(s) found");
  

• ut_assertok(run_command("bootflow list", 0));
• ut_assert_nextlinen("Showing all");
• ut_assert_nextlinen("Seq");
• ut_assert_nextlinen("---");
• ut_assert_nextlinen(" 0 extlinux");
• ut_assert_nextlinen(

• "  1  efi          ready   usb_mass_    1  usb_mass_storage.lun0.boo /EFI/BOOT/BOOTSBOX.EFI");
  

• ut_assert_nextlinen("---");
• ut_assert_skip_to_line("(2 bootflows, 2 valid)");
• ut_assert_console_end();
• ut_assertok(run_command("bootflow select 1", 0));
• ut_assert_console_end();
• /* signal to helloworld to exit boot services */

What 'helloworld' are you relating to?

Best regards

Heinrich

• systab.fw_vendor = test_vendor;
• ut_asserteq(1, run_command("bootflow boot", 0));
• ut_assert_nextline(

• "** Booting bootflow 'usb_mass_storage.lun0.bootdev.part_1' with efi");
  

• if (IS_ENABLED(CONFIG_LOGF_FUNC))

• ut_assert_skip_to_line("       efi_run_image() Booting /\\EFI\\BOOT\\BOOTSBOX.EFI");
  

• else

• ut_assert_skip_to_line("Booting /\\EFI\\BOOT\\BOOTSBOX.EFI");
  

• /* TODO: Why the \r ? */
• ut_assert_nextline("U-Boot test app for EFI_LOADER\r");
• ut_assert_nextline("Exiting boot sevices");
• if (IS_ENABLED(CONFIG_LOGF_FUNC))

• ut_assert_nextline("     do_bootefi_exec() ## Application failed, r = 5");
  

• else

• ut_assert_nextline("## Application failed, r = 5");
  

• ut_assert_nextline("Boot failed (err=-22)");
• ut_assert_console_end();
• return 0;

+} +BOOTSTD_TEST(bootflow_efi, UTF_CONSOLE);