On Tue, 21 Nov 2023 at 13:36, Simon Glass sjg@chromium.org wrote:

At present EFI output to the console use fputs() which bypasses the

nit, use ->uses

console-recording feature. This makes it impossible for tests to check the output of an EFI app.

There doesn't seem to be any need to do this bypass, so adjust it to simply use the puts() function.

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

Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

---

lib/efi_loader/efi_console.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index a2d137d7a9e1..28087582e8d6 100644 --- a/lib/efi_loader/efi_console.c +++ b/lib/efi_loader/efi_console.c @@ -182,7 +182,7 @@ static efi_status_t EFIAPI efi_cout_output_string( } pos = buf; utf16_utf8_strcpy(&pos, string);

•   fputs(stdout, buf);
  

•   puts(buf);
    free(buf);
  
    /*
  

-- 2.43.0.rc1.413.gea7ed67945-goog

On 11/21/23 12:35, Simon Glass wrote:

Show the vendor name since it useful to see that.

With this commit message it remains unclear why displaying the constant value (u"Das U-Boot") is of any value.

Best regards

Heinrich

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

lib/efi_loader/helloworld.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/lib/efi_loader/helloworld.c b/lib/efi_loader/helloworld.c index bd72822c0b72..1135d3a3c37e 100644 --- a/lib/efi_loader/helloworld.c +++ b/lib/efi_loader/helloworld.c @@ -234,6 +234,9 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle, (con_out, u"Missing device path for device handle\r\n"); goto out; }

• con_out->output_string(con_out, u"Vendor: ");
• con_out->output_string(con_out, systab->fw_vendor);
• con_out->output_string(con_out, u"\n"); con_out->output_string(con_out, u"Boot device: "); ret = print_device_path(device_path, device_path_to_text); if (ret != EFI_SUCCESS)

On 11/21/23 12:35, Simon Glass wrote:

Create a new disk for use with test, which contains the sandbox helloworld app. This will be used to test the EFI boot bootmeth.

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

test/py/tests/test_ut.py | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)

diff --git a/test/py/tests/test_ut.py b/test/py/tests/test_ut.py index 1d9149a3f683..f6220c05238a 100644 --- a/test/py/tests/test_ut.py +++ b/test/py/tests/test_ut.py @@ -468,6 +468,37 @@ 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"""
• mmc_dev = 7
• fname, mnt = setup_image(cons, mmc_dev, 0xc, second_part=True)

If you would set the partition type GUID to ESP, U-Boot would stop to complain about EFI variable that cannot be persisted.

There is a pending patch that wants to make the failure to persist EFI variable fatal.

[RESEND,v2] efi_loader: Fix UEFI variable error handling https://patchwork.ozlabs.org/project/uboot/patch/20231113161031.138304-1-o45...

Best regards

Heinrich

• loop = None
• mounted = 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/helloworld.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())
  

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

• @pytest.mark.buildconfigspec('cmd_bootflow') def test_ut_dm_init_bootstd(u_boot_console): """Initialise data for bootflow tests"""

@@ -476,6 +507,7 @@ def test_ut_dm_init_bootstd(u_boot_console): setup_bootmenu_image(u_boot_console) setup_cedit_file(u_boot_console) setup_cros_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 Tue, 21 Nov 2023 at 10:32, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 11/21/23 12:35, Simon Glass wrote:

...

We don't want ANSI characters written in tests since it is a pain to check the output.

I never experienced such pain. The Gitlab output just renders things as expected.

...

For now, don't bother checking the console size.

A better solution is need, which allows ANSI output to be controlled within EFI.

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

include/efi_loader.h | 14 ++++++++++++-- lib/efi_loader/efi_console.c | 17 +++++++---------- lib/efi_loader/efi_setup.c | 2 +- 3 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h index 247be060e1c0..ebe951922791 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -527,8 +527,18 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void); efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index); /* 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 */ -void efi_setup_console_size(void);

+/**

• • efi_setup_console_size() - update the mode table.
• • @allow_ansi: Allow emitting ANSI characters
• • 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(bool allow_ansi);

• /* Install device tree */ efi_status_t efi_install_fdt(void *fdt); /* Run loaded UEFI image */

diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index 28087582e8d6..ea1608f8e6ec 100644 --- a/lib/efi_loader/efi_console.c +++ b/lib/efi_loader/efi_console.c @@ -349,22 +349,19 @@ 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) +void efi_setup_console_size(bool allow_ansi) { int rows = 25, cols = 80; int ret = -ENODEV;

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

• if (ret)
  

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

• if (ret) {
  

•         if (allow_ansi)
  

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

•         else
  

•                 ret = 0;
  

• }
  if (ret)
          return;
  
  

diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c index e6de685e8795..d4226a3011ac 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -230,7 +230,7 @@ efi_status_t efi_init_obj_list(void) return efi_obj_list_initialized;

  /* Set up console modes */

• efi_setup_console_size();
  

• efi_setup_console_size(false);
  

This would render our console non-compliant. Please, keep it as is.

Testing is not the primary task of U-Boot. If you need to filter test output, please, add a filter method to your test setup.

This is just a WIP patch, but we do need a way to avoid outputting ANSI output since ut_assert_nextline() needs that. Can you suggest a better way? Perhaps we could have some EFI settings somewhere?

Regards, Simon

Hi Heinrich,

On Tue, 21 Nov 2023 at 10:17, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 11/21/23 12:35, Simon Glass wrote:

...

This allows testing of the exit_boot_services call, providing more coverage of the EFI bootmeth.

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

I would prefer to keep helloworld.efi benign.

Please, create a new EFI binary for testing ExitBootServices().

Eek...isn't helloworld already used in tets?

...

---

lib/efi_loader/helloworld.c | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/helloworld.c b/lib/efi_loader/helloworld.c index 1135d3a3c37e..1fb5fb5a62f2 100644 --- a/lib/efi_loader/helloworld.c +++ b/lib/efi_loader/helloworld.c @@ -206,6 +206,26 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle, (con_out, u"Cannot open loaded image protocol\r\n"); goto out; }

• {
  

•         ulong ptr = (ulong)loaded_image;
  

•         u16 str[80];
  

•         int i;
  

•         for (i = 0; i < 8; i++) {
  

•                 uint digit = (ptr >> ((7 - i) * 4)) & 0xf;
  

•                 if (digit > 9)
  

•                         digit = 'a' + digit - 10;
  

•                 else
  

•                         digit += '0';
  

•                 str[i] = digit;
  

•         }
  

•         str[i] = 0;
  

•         con_out->output_string(con_out, str);
  

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

• }
  

• print_load_options(loaded_image);
  
  /* Get the device path to text protocol */
  

@@ -243,7 +263,21 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle, goto out;

out:

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

• /*
  

•  * TODO: Use vendor string to decide whether to call exit-boot-services
  

•  */
  

• efi_uintn_t map_size = 0;
  

• efi_uintn_t map_key;
  

• efi_uintn_t desc_size;
  

• u32 desc_version;
  

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

• boottime->exit_boot_services(handle, map_key);
  

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

After ExitBootServices() you must not call boot services.

Call runtime->reset_system(EFI_RESET_COLD, EFI_SUCCESS, 0, NULL) instead.

OK, will do. That will be ignored by sandbox, right?

For now this is just a hack to try to resolve the problem mentioned.

Regards, Simon

On 11/21/23 12:35, Simon Glass wrote:

This works when run outside a test:

good:

/tmp/b/sandbox/u-boot -T -c "bootfl scan mmc; bootfl sel 1; bootfl list; bootfl b"

** Booting bootflow 'mmc7.bootdev.part_1' with efi desc = 0000000018c86ee0 dev=mmc, devnr=7:1, path=efi/boot/bootsbox.efi, buffer=0000000011001000, size=1600

• boot device /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,0000000000000000)/SD(7)/SD(3)/HD(1,MBR,0x2f7d9756,0x800,0x9000)
• image /efi\boot\bootsbox.efi

No EFI system partition No EFI system partition Failed to persist EFI variables EFI using ACPI tables at 8ef8000 WARNING: Can't have ACPI table and device tree - ignoring DT. Loaded from disk Booting /efi\boot\bootsbox.efi EFI: Call: efi_load_image(false, efi_root, file_path, source_buffer, source_size, &handle) info->device_handle = 0000000018f180e0 loaded_image info for 0000000018f184e0: 0000000018f18470

EFI: 0 returned by efi_load_image(false, efi_root, file_path, source_buffer, source_size, &handle) EFI: Call: efi_start_image(handle, &exit_data_size, &exit_data) Hello, world! Running on UEFI 2.10 Have ACPI 2.0 table Have SMBIOS table 18f18470 Load options: <none> finding

• search 0000000018c931a0 ret=0

File path: /efi\boot\bootsbox.efi Vendor: Das U-Boot Boot device: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,0000000000000000)/SD(7)/SD(3)/HD(1,MBR,0x2f7d9756,0x800,0x9000) Exiting boot sevices EFI: 5 returned by efi_start_image(handle, &exit_data_size, &exit_data) Boot failed (err=-22)

bad:

rtv bootflow_efi

** Booting bootflow 'mmc7.bootdev.part_1' with efi

Should we have an ESP on the MMC image to persist UEFI variables?

Best regards

Heinrich

desc = 0000000018c86ee0 dev=mmc, devnr=8:1, path=efi/boot/bootsbox.efi, buffer=0000000011001000, size=1600

• boot device /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,0000000000000000)/SD(7)/SD(3)/HD(1,MBR,0x2f7d9756,0x800,0x9000)
• image /efi\boot\bootsbox.efi

No EFI system partition No EFI system partition Failed to persist EFI variables EFI using ACPI tables at 8ef8000 WARNING: Can't have ACPI table and device tree - ignoring DT. Loaded from disk Booting /efi\boot\bootsbox.efi EFI: Call: efi_load_image(false, efi_root, file_path, source_buffer, source_size, &handle) info->device_handle = 0000000000000000 loaded_image info for 0000000018c924e0: 0000000018f15080

EFI: 0 returned by efi_load_image(false, efi_root, file_path, source_buffer, source_size, &handle) EFI: Call: efi_start_image(handle, &exit_data_size, &exit_data) Hello, world! Running on UEFI 2.10 Have ACPI 2.0 table Have SMBIOS table 18f15080 Load options: <none> finding

• search 0000000018c931a0 ret=0

File path: /efi\boot\bootsbox.efi Missing device handle Exiting boot sevices EFI: 5 returned by efi_start_image(handle, &exit_data_size, &exit_data) Boot failed (err=-22) test/boot/bootflow.c:1091, bootflow_efi(): console: Expected 'No EFI system partition', got 'desc = 0000000018c86ee0' Test bootflow_efi failed 1 times Failures: 1

(see 'Missing device handle')

It seems that the device number is inconsistent between generation and decoding.

function rt_get_suite_and_name() { local arg #echo arg $arg suite= name=

if [ "$1" = "-f" ]; then force="-f" shift fi arg="$1" rest="$2"

# The symbol is something like this: # _u_boot_list_2_ut_bootstd_test_2_vbe_simple_test_base # Split it into the suite name (bootstd) and test name # (vbe_simple_test_base) read suite name < \ <(nm /tmp/b/$exec/u-boot |grep "list_2_ut.*$arg.*" \ | cut -d' ' -f3 \ | head -1 \ | sed -n 's/_u_boot_list_2_ut_(.*)_test_2_/\1 /p') #echo suite $suite #echo name $name #name=${1#dm_test_} #name=${name#ut_dm_} }

function rtv() { local exec=sandbox local suite name force rest rt_get_suite_and_name $*

U_BOOT_PERSISTENT_DATA_DIR=/tmp/b/sandbox/persistent-data \ /tmp/b/$exec/u-boot -v -T $rest -c "ut $suite $force $name" }

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

boot/bootflow.c | 2 ++ boot/bootmeth_efi.c | 1 + cmd/bootefi.c | 1 + lib/efi_loader/efi_boottime.c | 5 +++++ test/boot/bootflow.c | 2 +- 5 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/boot/bootflow.c b/boot/bootflow.c index 6922e7e0c4e7..0513522a7ec4 100644 --- a/boot/bootflow.c +++ b/boot/bootflow.c @@ -464,6 +464,8 @@ void bootflow_init(struct bootflow *bflow, struct udevice *bootdev,

void bootflow_free(struct bootflow *bflow) {

• /* this is where we want to get to (will only happen with USB) */
• printf("bootflow free\n"); free(bflow->name); free(bflow->subdir); free(bflow->fname);

diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c index ae936c8daa18..edee9e62a458 100644 --- a/boot/bootmeth_efi.c +++ b/boot/bootmeth_efi.c @@ -127,6 +127,7 @@ static void set_efi_bootdev(struct blk_desc *desc, struct bootflow *bflow) * this can go away. */ media_dev = dev_get_parent(bflow->dev);

• printf("desc = %p\n", desc); snprintf(devnum_str, sizeof(devnum_str), "%x:%x", desc ? desc->devnum : dev_seq(media_dev), bflow->part);

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 20e5c94a33a4..3c014e5684aa 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -5,6 +5,7 @@

• Copyright (c) 2016 Alexander Graf

*/

+#define LOG_DEBUG #define LOG_CATEGORY LOGC_EFI

#include <common.h> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 0b7579cb5af1..149da5798740 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1820,6 +1820,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,

if (device_path) { info->device_handle = efi_dp_find_obj(device_path, NULL, NULL);

• printf("info->device_handle = %p\n", info->device_handle);
  
  

  dp = efi_dp_append(device_path, file_path); if (!dp) {

@@ -1828,6 +1829,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path, } } else { dp = NULL;

• printf("\nno device handle\n");
  

  } ret = efi_add_protocol(&obj->header, &efi_guid_loaded_image_device_path, dp);

@@ -1842,6 +1844,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path, &efi_guid_loaded_image, info); if (ret != EFI_SUCCESS) goto failure;

• printf("loaded_image info for %p: %p\n\n", &obj->header, info);

  *info_ptr = info; *handle_ptr = obj;

@@ -2665,8 +2668,10 @@ static efi_status_t EFIAPI efi_locate_protocol(const efi_guid_t *protocol, if (ret == EFI_SUCCESS) goto found; } else {

• printf("finding\n");
  

  list_for_each_entry(efiobj, &efi_obj_list, link) { ret = efi_search_protocol(efiobj, protocol, &handler);

• 	printf("- search %p ret=%lx\n", efiobj, ret);
  if (ret == EFI_SUCCESS)
  	goto found;
  

  }

diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index ad54ef6eaabc..76c65c46cc01 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -536,7 +536,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));