[PATCH v6 00/12] efi: Add a test for EFI bootmeth
The test coverage for the EFI bootmeth is incomplete since it does not actually boot the application.
This series creates a simple test for this purpose. It includes a few patches to make this work:
- ANSI output from the EFI loader confusing the unit-testing checker - Hang in sandbox virtio due to EFI probing all block devices
Other necessary fixes have been split out into two other series.
Changes in v6: - Expand the debug messages to be more descriptive - Drop the patch to disable sandbox virtio blk with EFI - Add new patch to disable the sandbox virtio blk device - Deal with sandbox CONFIG_LOGF_FUNC - Rebase on -next - Drop patches previously applied - Drop mention of helloworld since it is no-longer used by this test
Changes in v5: - Drop the Fixes tag
Changes in v4: - Add efi_loader tag to some patches - Split out non-EFI patches into a different series
Changes in v3: - Drop the extra- rules since scripts/Makefile.lib takes care of it - Add new patch to drop crt0/relocal extra- rules - Put back the Linaro copyright accidentally removed
Changes in v2: - Reword commit message - Use 'Firmware vendor' instead of just 'Vendor' - Add many new patches to resolve all the outstanding test issues
Simon Glass (12): efi_loader: Rename and move CMD_BOOTEFI_HELLO_COMPILE efi: arm: x86: riscv: Drop crt0/relocal extra- rules efi_loader: Shorten the app rules efi_loader: Shorten the app rules further efi_loader: Show the vendor in helloworld efi: Use the same filename for all sandbox builds bootstd: Add debugging for efi bootmeth efi_loader: Disable ANSI output for tests efi_loader: Add a test app sandbox: virtio: Disable the sandbox virtio blk device test: efi: boot: Set up an image suitable for EFI testing test: efi: boot: Add a test for the efi bootmeth
arch/Kconfig | 3 +- arch/arm/lib/Makefile | 8 ---- arch/riscv/lib/Makefile | 4 -- arch/sandbox/dts/test.dts | 2 +- arch/x86/lib/Makefile | 16 ------- boot/bootmeth_efi.c | 11 ++++- cmd/Kconfig | 14 +----- configs/octeontx2_95xx_defconfig | 2 +- configs/octeontx2_96xx_defconfig | 2 +- configs/octeontx_81xx_defconfig | 2 +- configs/octeontx_83xx_defconfig | 2 +- doc/develop/uefi/uefi.rst | 2 +- include/efi_default_filename.h | 24 +--------- include/efi_loader.h | 21 ++++++++- lib/efi_loader/Kconfig | 22 +++++++++ lib/efi_loader/Makefile | 47 ++++++------------- lib/efi_loader/efi_console.c | 26 +++++++---- lib/efi_loader/helloworld.c | 3 ++ lib/efi_loader/testapp.c | 68 ++++++++++++++++++++++++++++ test/boot/bootdev.c | 18 +++++++- test/boot/bootflow.c | 65 +++++++++++++++++++++++++- test/py/tests/bootstd/flash1.img.xz | Bin 0 -> 5016 bytes test/py/tests/test_efi_fit.py | 2 +- test/py/tests/test_efi_loader.py | 2 +- test/py/tests/test_ut.py | 52 ++++++++++++++++++--- 25 files changed, 294 insertions(+), 124 deletions(-) create mode 100644 lib/efi_loader/testapp.c create mode 100644 test/py/tests/bootstd/flash1.img.xz
This is not actually a command so the name is confusing. Use BOOTEFI_HELLO_COMPILE instead. Put it in the efi_loader directory with the other such config options.
The link rule (for $(obj)/%_efi.so) in scripts/Makefile.lib handles pulling in efi_crt0.o and efi_reloc.o so drop the 'extra' rules.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org ---
(no changes since v3)
Changes in v3: - Drop the extra- rules since scripts/Makefile.lib takes care of it
arch/arm/lib/Makefile | 1 - arch/riscv/lib/Makefile | 1 - arch/x86/lib/Makefile | 2 +- cmd/Kconfig | 14 +------------- configs/octeontx2_95xx_defconfig | 2 +- configs/octeontx2_96xx_defconfig | 2 +- configs/octeontx_81xx_defconfig | 2 +- configs/octeontx_83xx_defconfig | 2 +- doc/develop/uefi/uefi.rst | 2 +- lib/efi_loader/Kconfig | 12 ++++++++++++ lib/efi_loader/Makefile | 2 +- test/py/tests/test_efi_fit.py | 2 +- test/py/tests/test_efi_loader.py | 2 +- 13 files changed, 22 insertions(+), 24 deletions(-)
diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile index 67275fba616..4e54c577cc3 100644 --- a/arch/arm/lib/Makefile +++ b/arch/arm/lib/Makefile @@ -130,7 +130,6 @@ CFLAGS_REMOVE_$(EFI_CRT0) := $(CFLAGS_NON_EFI) CFLAGS_$(EFI_RELOC) := $(CFLAGS_EFI) CFLAGS_REMOVE_$(EFI_RELOC) := $(CFLAGS_NON_EFI)
-extra-$(CONFIG_CMD_BOOTEFI_HELLO_COMPILE) += $(EFI_CRT0) $(EFI_RELOC) # TODO: As of v2019.01 the relocation code for the EFI application cannot # be built on ARMv7-M. ifndef CONFIG_CPU_V7M diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile index 65dc49f6fa5..4f6272aab6e 100644 --- a/arch/riscv/lib/Makefile +++ b/arch/riscv/lib/Makefile @@ -36,7 +36,6 @@ CFLAGS_REMOVE_$(EFI_CRT0) := $(CFLAGS_NON_EFI) CFLAGS_$(EFI_RELOC) := $(CFLAGS_EFI) CFLAGS_REMOVE_$(EFI_RELOC) := $(CFLAGS_NON_EFI)
-extra-$(CONFIG_CMD_BOOTEFI_HELLO_COMPILE) += $(EFI_CRT0) $(EFI_RELOC) extra-$(CONFIG_CMD_BOOTEFI_SELFTEST) += $(EFI_CRT0) $(EFI_RELOC) extra-$(CONFIG_EFI) += $(EFI_CRT0) $(EFI_RELOC)
diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile index 8fc35e1b51e..8bc8d92172b 100644 --- a/arch/x86/lib/Makefile +++ b/arch/x86/lib/Makefile @@ -97,7 +97,7 @@ endif else
ifndef CONFIG_SPL_BUILD -ifneq ($(CONFIG_CMD_BOOTEFI_SELFTEST)$(CONFIG_CMD_BOOTEFI_HELLO_COMPILE),) +ifneq ($(CONFIG_CMD_BOOTEFI_SELFTEST),) extra-y += $(EFI_CRT0) $(EFI_RELOC) endif endif diff --git a/cmd/Kconfig b/cmd/Kconfig index 5ef3c8a8748..74afedd5b5a 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -438,21 +438,9 @@ config CMD_BOOTEFI_BOOTMGR This subcommand will allow you to select the UEFI binary to be booted via UEFI variables Boot####, BootOrder, and BootNext.
-config CMD_BOOTEFI_HELLO_COMPILE - bool "Compile a standard EFI hello world binary for testing" - default y - help - This compiles a standard EFI hello world application with U-Boot so - that it can be used with the test/py testing framework. This is useful - for testing that EFI is working at a basic level, and for bringing - up EFI support on a new architecture. - - No additional space will be required in the resulting U-Boot binary - when this option is enabled. - config CMD_BOOTEFI_HELLO bool "Allow booting a standard EFI hello world for testing" - depends on CMD_BOOTEFI_BINARY && CMD_BOOTEFI_HELLO_COMPILE + depends on CMD_BOOTEFI_BINARY && BOOTEFI_HELLO_COMPILE default y if CMD_BOOTEFI_SELFTEST help This adds a standard EFI hello world application to U-Boot so that diff --git a/configs/octeontx2_95xx_defconfig b/configs/octeontx2_95xx_defconfig index c5dc4f4dfa6..23c313375ac 100644 --- a/configs/octeontx2_95xx_defconfig +++ b/configs/octeontx2_95xx_defconfig @@ -38,7 +38,7 @@ CONFIG_SYS_PBSIZE=1050 CONFIG_BOARD_EARLY_INIT_R=y CONFIG_HUSH_PARSER=y CONFIG_SYS_PROMPT="Marvell> " -# CONFIG_CMD_BOOTEFI_HELLO_COMPILE is not set +# CONFIG_BOOTEFI_HELLO_COMPILE is not set CONFIG_CMD_MD5SUM=y CONFIG_MD5SUM_VERIFY=y CONFIG_CMD_MX_CYCLIC=y diff --git a/configs/octeontx2_96xx_defconfig b/configs/octeontx2_96xx_defconfig index ad61b80300f..197e72acd1f 100644 --- a/configs/octeontx2_96xx_defconfig +++ b/configs/octeontx2_96xx_defconfig @@ -38,7 +38,7 @@ CONFIG_SYS_PBSIZE=1050 CONFIG_BOARD_EARLY_INIT_R=y CONFIG_HUSH_PARSER=y CONFIG_SYS_PROMPT="Marvell> " -# CONFIG_CMD_BOOTEFI_HELLO_COMPILE is not set +# CONFIG_BOOTEFI_HELLO_COMPILE is not set CONFIG_CMD_MD5SUM=y CONFIG_MD5SUM_VERIFY=y CONFIG_CMD_MX_CYCLIC=y diff --git a/configs/octeontx_81xx_defconfig b/configs/octeontx_81xx_defconfig index 1d39bce6abd..b501d653c27 100644 --- a/configs/octeontx_81xx_defconfig +++ b/configs/octeontx_81xx_defconfig @@ -39,7 +39,7 @@ CONFIG_SYS_PBSIZE=1050 CONFIG_BOARD_EARLY_INIT_R=y CONFIG_HUSH_PARSER=y CONFIG_SYS_PROMPT="Marvell> " -# CONFIG_CMD_BOOTEFI_HELLO_COMPILE is not set +# CONFIG_BOOTEFI_HELLO_COMPILE is not set CONFIG_CMD_MD5SUM=y CONFIG_MD5SUM_VERIFY=y CONFIG_CMD_MX_CYCLIC=y diff --git a/configs/octeontx_83xx_defconfig b/configs/octeontx_83xx_defconfig index ba9fc5f9553..4a537f5cbbe 100644 --- a/configs/octeontx_83xx_defconfig +++ b/configs/octeontx_83xx_defconfig @@ -37,7 +37,7 @@ CONFIG_SYS_PBSIZE=1050 CONFIG_BOARD_EARLY_INIT_R=y CONFIG_HUSH_PARSER=y CONFIG_SYS_PROMPT="Marvell> " -# CONFIG_CMD_BOOTEFI_HELLO_COMPILE is not set +# CONFIG_BOOTEFI_HELLO_COMPILE is not set CONFIG_CMD_MD5SUM=y CONFIG_MD5SUM_VERIFY=y CONFIG_CMD_MX_CYCLIC=y diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst index 94482758573..0760ca91d4f 100644 --- a/doc/develop/uefi/uefi.rst +++ b/doc/develop/uefi/uefi.rst @@ -720,7 +720,7 @@ Executing the built in hello world application
A hello world UEFI application can be built with::
- CONFIG_CMD_BOOTEFI_HELLO_COMPILE=y + CONFIG_BOOTEFI_HELLO_COMPILE=y
It can be embedded into the U-Boot binary with::
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index e58b8825605..6f6fa8d629d 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -552,6 +552,18 @@ config EFI_HTTP_BOOT directly boot from network. endmenu
+config BOOTEFI_HELLO_COMPILE + bool "Compile a standard EFI hello world binary for testing" + default y + help + This compiles a standard EFI hello world application with U-Boot so + that it can be used with the test/py testing framework. This is useful + for testing that EFI is working at a basic level, and for bringing + up EFI support on a new architecture. + + No additional space will be required in the resulting U-Boot binary + when this option is enabled. + endif
source "lib/efi/Kconfig" diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile index 2af6f2066b5..27dbd9e760d 100644 --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -27,7 +27,7 @@ always += boothart.efi targets += boothart.o endif
-ifneq ($(CONFIG_CMD_BOOTEFI_HELLO_COMPILE),) +ifneq ($(CONFIG_BOOTEFI_HELLO_COMPILE),) always += helloworld.efi targets += helloworld.o endif diff --git a/test/py/tests/test_efi_fit.py b/test/py/tests/test_efi_fit.py index 0ad483500f8..550058a30fd 100644 --- a/test/py/tests/test_efi_fit.py +++ b/test/py/tests/test_efi_fit.py @@ -119,7 +119,7 @@ FDT_DATA = ''' '''
@pytest.mark.buildconfigspec('bootm_efi') -@pytest.mark.buildconfigspec('cmd_bootefi_hello_compile') +@pytest.mark.buildconfigspec('BOOTEFI_HELLO_COMPILE') @pytest.mark.buildconfigspec('fit') @pytest.mark.notbuildconfigspec('generate_acpi_table') @pytest.mark.requiredtool('dtc') diff --git a/test/py/tests/test_efi_loader.py b/test/py/tests/test_efi_loader.py index 5f3b448a066..707b2c9e795 100644 --- a/test/py/tests/test_efi_loader.py +++ b/test/py/tests/test_efi_loader.py @@ -170,7 +170,7 @@ def do_test_efi_helloworld_net(u_boot_console, proto): assert expected_text not in output
@pytest.mark.buildconfigspec('of_control') -@pytest.mark.buildconfigspec('cmd_bootefi_hello_compile') +@pytest.mark.buildconfigspec('bootefi_hello_compile') @pytest.mark.buildconfigspec('cmd_tftpboot') def test_efi_helloworld_net_tftp(u_boot_console): """Run the helloworld.efi binary via TFTP.
The link rule (for $(obj)/%_efi.so) in scripts/Makefile.lib handles pulling in efi_crt0.o and efi_reloc.o so drop the 'extra' rules.
Signed-off-by: Simon Glass sjg@chromium.org Suggested-by: Heinrich Schuchardt xypron.glpk@gmx.de ---
(no changes since v3)
Changes in v3: - Add new patch to drop crt0/relocal extra- rules
arch/arm/lib/Makefile | 7 ------- arch/riscv/lib/Makefile | 3 --- arch/x86/lib/Makefile | 16 ---------------- 3 files changed, 26 deletions(-)
diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile index 4e54c577cc3..87000d1609b 100644 --- a/arch/arm/lib/Makefile +++ b/arch/arm/lib/Makefile @@ -129,10 +129,3 @@ CFLAGS_REMOVE_$(EFI_CRT0) := $(CFLAGS_NON_EFI)
CFLAGS_$(EFI_RELOC) := $(CFLAGS_EFI) CFLAGS_REMOVE_$(EFI_RELOC) := $(CFLAGS_NON_EFI) - -# TODO: As of v2019.01 the relocation code for the EFI application cannot -# be built on ARMv7-M. -ifndef CONFIG_CPU_V7M -#extra-$(CONFIG_CMD_BOOTEFI_SELFTEST) += $(EFI_CRT0) $(EFI_RELOC) -endif -extra-$(CONFIG_EFI) += $(EFI_CRT0) $(EFI_RELOC) diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile index 4f6272aab6e..bcfdb516b76 100644 --- a/arch/riscv/lib/Makefile +++ b/arch/riscv/lib/Makefile @@ -36,9 +36,6 @@ CFLAGS_REMOVE_$(EFI_CRT0) := $(CFLAGS_NON_EFI) CFLAGS_$(EFI_RELOC) := $(CFLAGS_EFI) CFLAGS_REMOVE_$(EFI_RELOC) := $(CFLAGS_NON_EFI)
-extra-$(CONFIG_CMD_BOOTEFI_SELFTEST) += $(EFI_CRT0) $(EFI_RELOC) -extra-$(CONFIG_EFI) += $(EFI_CRT0) $(EFI_RELOC) - obj-$(CONFIG_$(SPL_TPL_)USE_ARCH_MEMSET) += memset.o obj-$(CONFIG_$(SPL_TPL_)USE_ARCH_MEMMOVE) += memmove.o obj-$(CONFIG_$(SPL_TPL_)USE_ARCH_MEMCPY) += memcpy.o diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile index 8bc8d92172b..d6ea9c955f8 100644 --- a/arch/x86/lib/Makefile +++ b/arch/x86/lib/Makefile @@ -87,19 +87,3 @@ extra-$(CONFIG_EFI_STUB_32BIT) += crt0_ia32_efi.o reloc_ia32_efi.o extra-$(CONFIG_EFI_STUB_64BIT) += crt0_x86_64_efi.o reloc_x86_64_efi.o
endif - -ifdef CONFIG_EFI_STUB - -ifeq ($(CONFIG_$(SPL_)X86_64),) -extra-y += $(EFI_CRT0) $(EFI_RELOC) -endif - -else - -ifndef CONFIG_SPL_BUILD -ifneq ($(CONFIG_CMD_BOOTEFI_SELFTEST),) -extra-y += $(EFI_CRT0) $(EFI_RELOC) -endif -endif - -endif
We have quite a few apps now, so create a way to specify them as a list rather than repeating the same rules again and again.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org ---
(no changes since v1)
lib/efi_loader/Makefile | 31 +++++++++---------------------- 1 file changed, 9 insertions(+), 22 deletions(-)
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile index 27dbd9e760d..660368b9d8f 100644 --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -22,29 +22,13 @@ CFLAGS_REMOVE_dtbdump.o := $(CFLAGS_NON_EFI) CFLAGS_initrddump.o := $(CFLAGS_EFI) -Os -ffreestanding CFLAGS_REMOVE_initrddump.o := $(CFLAGS_NON_EFI)
-ifdef CONFIG_RISCV -always += boothart.efi -targets += boothart.o -endif - -ifneq ($(CONFIG_BOOTEFI_HELLO_COMPILE),) -always += helloworld.efi -targets += helloworld.o -endif - -ifneq ($(CONFIG_GENERATE_SMBIOS_TABLE),) -always += smbiosdump.efi -targets += smbiosdump.o -endif - +# These are the apps that are built +apps-$(CONFIG_RISCV) += boothart +apps-$(CONFIG_BOOTEFI_HELLO_COMPILE) += helloworld +apps-$(CONFIG_GENERATE_SMBIOS_TABLE) += smbiosdump +apps-$(CONFIG_EFI_LOAD_FILE2_INITRD) += initrddump ifeq ($(CONFIG_GENERATE_ACPI_TABLE),) -always += dtbdump.efi -targets += dtbdump.o -endif - -ifdef CONFIG_EFI_LOAD_FILE2_INITRD -always += initrddump.efi -targets += initrddump.o +apps-y += dtbdump endif
obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o @@ -95,3 +79,6 @@ obj-$(CONFIG_EFI_ECPT) += efi_conformance.o
EFI_VAR_SEED_FILE := $(subst $",,$(CONFIG_EFI_VAR_SEED_FILE)) $(obj)/efi_var_seed.o: $(srctree)/$(EFI_VAR_SEED_FILE) + +always += $(foreach f,$(apps-y),$(f).efi) +targets += $(foreach f,$(apps-y),$(f).o)
Add a way to factor out the CFLAGS changes for each app, since they are all the same.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org ---
(no changes since v1)
lib/efi_loader/Makefile | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile index 660368b9d8f..00d18966f9e 100644 --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -11,16 +11,6 @@ asflags-y += -I. CFLAGS_efi_boottime.o += \ -DFW_VERSION="0x$(VERSION)" \ -DFW_PATCHLEVEL="0x$(PATCHLEVEL)" -CFLAGS_boothart.o := $(CFLAGS_EFI) -Os -ffreestanding -CFLAGS_REMOVE_boothart.o := $(CFLAGS_NON_EFI) -CFLAGS_helloworld.o := $(CFLAGS_EFI) -Os -ffreestanding -CFLAGS_REMOVE_helloworld.o := $(CFLAGS_NON_EFI) -CFLAGS_smbiosdump.o := $(CFLAGS_EFI) -Os -ffreestanding -CFLAGS_REMOVE_smbiosdump.o := $(CFLAGS_NON_EFI) -CFLAGS_dtbdump.o := $(CFLAGS_EFI) -Os -ffreestanding -CFLAGS_REMOVE_dtbdump.o := $(CFLAGS_NON_EFI) -CFLAGS_initrddump.o := $(CFLAGS_EFI) -Os -ffreestanding -CFLAGS_REMOVE_initrddump.o := $(CFLAGS_NON_EFI)
# These are the apps that are built apps-$(CONFIG_RISCV) += boothart @@ -80,5 +70,10 @@ obj-$(CONFIG_EFI_ECPT) += efi_conformance.o EFI_VAR_SEED_FILE := $(subst $",,$(CONFIG_EFI_VAR_SEED_FILE)) $(obj)/efi_var_seed.o: $(srctree)/$(EFI_VAR_SEED_FILE)
+# Set the C flags to add and remove for each app +$(foreach f,$(apps-y),\ + $(eval CFLAGS_$(f).o := $(CFLAGS_EFI) -Os -ffreestanding)\ + $(eval CFLAGS_REMOVE_$(f).o := $(CFLAGS_NON_EFI))) + always += $(foreach f,$(apps-y),$(f).efi) targets += $(foreach f,$(apps-y),$(f).o)
Show the vendor name so it is clear which firmware is being used, e.g. whether U-Boot is providing the boot services.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v2)
Changes in v2: - Reword commit message - Use 'Firmware vendor' instead of just 'Vendor'
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 586177de0c8..c4d2afcb40a 100644 --- a/lib/efi_loader/helloworld.c +++ b/lib/efi_loader/helloworld.c @@ -237,6 +237,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"Firmware 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)
Hi Simon
On Fri, 27 Sept 2024 at 01:01, Simon Glass sjg@chromium.org wrote:
Show the vendor name so it is clear which firmware is being used, e.g. whether U-Boot is providing the boot services.
Signed-off-by: Simon Glass sjg@chromium.org
I already sent my r-b for this. [0]. I am not sure Heinrich remarks were covered
(no changes since v2)
Changes in v2:
- Reword commit message
- Use 'Firmware vendor' instead of just 'Vendor'
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 586177de0c8..c4d2afcb40a 100644 --- a/lib/efi_loader/helloworld.c +++ b/lib/efi_loader/helloworld.c @@ -237,6 +237,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"Firmware 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)-- 2.43.0
[0] https://lore.kernel.org/u-boot/CAC_iWj+DtHbghAqJWiwm8ZVCXf3mCOOm13YpLSQGGBPc...
Hi Ilias,
On Fri, 27 Sept 2024 at 06:00, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon
On Fri, 27 Sept 2024 at 01:01, Simon Glass sjg@chromium.org wrote:
Show the vendor name so it is clear which firmware is being used, e.g. whether U-Boot is providing the boot services.
Signed-off-by: Simon Glass sjg@chromium.org
I already sent my r-b for this. [0]. I am not sure Heinrich remarks were covered
Heinrich basically didn't see the point of this patch, but then suggested printing out FirmwareRevision too. I did reply but didn't hear back.
So I suggest we just go with this and we can expand it later. Or just drop the patch.
(no changes since v2)
Changes in v2:
- Reword commit message
- Use 'Firmware vendor' instead of just 'Vendor'
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 586177de0c8..c4d2afcb40a 100644 --- a/lib/efi_loader/helloworld.c +++ b/lib/efi_loader/helloworld.c @@ -237,6 +237,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"Firmware 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)-- 2.43.0
[0] https://lore.kernel.org/u-boot/CAC_iWj+DtHbghAqJWiwm8ZVCXf3mCOOm13YpLSQGGBPc...
Regards, Simon
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.
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 ---
(no changes since v5)
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
On 26.09.24 23:59, 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.
Drop the confusing use of host architecture and just let sandbox be sandbox.
As I already wrote in https://lore.kernel.org/u-boot/ae1cf1fa-766e-46a0-8ef9-2c2c7af73d9e@gmx.de/ this patch should not be merged.
bootsbx.efi does not exist in the UEFI specification.
Without this patch I can test that shim work and grub are correctly loaded from a distro image. This patch makes the sandbox misbehave.
Best regards
Heinrich
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
(no changes since v5)
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 Mon, 30 Sept 2024 at 17:23, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 26.09.24 23:59, 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.
Drop the confusing use of host architecture and just let sandbox be sandbox.
As I already wrote in https://lore.kernel.org/u-boot/ae1cf1fa-766e-46a0-8ef9-2c2c7af73d9e@gmx.de/ this patch should not be merged.
bootsbx.efi does not exist in the UEFI specification.
If that is of concern I can get it added. Let me know.
Without this patch I can test that shim work and grub are correctly loaded from a distro image. This patch makes the sandbox misbehave.
Why don't you do that with QEMU?
If we want sandbox to do this, I could add a way for sandbox to select its architecture. But this patch is correct, sorry. It is basically a revert of your patch:
3a0654ecd0d efi_loader: correctly identify binary name
Let me know if you would like a selection mechanism and I'll see what I can do.
Regards, Simon
On Thu, Oct 17, 2024 at 05:23:19PM -0600, Simon Glass wrote:
Hi Heinrich,
On Mon, 30 Sept 2024 at 17:23, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 26.09.24 23:59, 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.
Drop the confusing use of host architecture and just let sandbox be sandbox.
As I already wrote in https://lore.kernel.org/u-boot/ae1cf1fa-766e-46a0-8ef9-2c2c7af73d9e@gmx.de/ this patch should not be merged.
bootsbx.efi does not exist in the UEFI specification.
If that is of concern I can get it added. Let me know.
Without this patch I can test that shim work and grub are correctly loaded from a distro image. This patch makes the sandbox misbehave.
Why don't you do that with QEMU?
If we want sandbox to do this, I could add a way for sandbox to select its architecture. But this patch is correct, sorry. It is basically a revert of your patch:
3a0654ecd0d efi_loader: correctly identify binary name
Let me know if you would like a selection mechanism and I'll see what I can do.
I'm confused. It sounds like Heinrich is using sandbox to test on various hosts, how U-Boot behaves, without relying on "just test in QEMU". I don't see how this disconnects from your usual request on how to use sandbox for further testing. What is breaking for you, Simon, by us saying that the host instruction set determines what we load and run (and so, is portable whereas saying "sandbox" is not, between x86 hosts, arm64 hosts and riscv64 hosts).
Regards, Simon
Hi Tom,
On Thu, 17 Oct 2024 at 18:17, Tom Rini trini@konsulko.com wrote:
On Thu, Oct 17, 2024 at 05:23:19PM -0600, Simon Glass wrote:
Hi Heinrich,
On Mon, 30 Sept 2024 at 17:23, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 26.09.24 23:59, 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.
Drop the confusing use of host architecture and just let sandbox be sandbox.
As I already wrote in https://lore.kernel.org/u-boot/ae1cf1fa-766e-46a0-8ef9-2c2c7af73d9e@gmx.de/ this patch should not be merged.
bootsbx.efi does not exist in the UEFI specification.
If that is of concern I can get it added. Let me know.
Without this patch I can test that shim work and grub are correctly loaded from a distro image. This patch makes the sandbox misbehave.
Why don't you do that with QEMU?
If we want sandbox to do this, I could add a way for sandbox to select its architecture. But this patch is correct, sorry. It is basically a revert of your patch:
3a0654ecd0d efi_loader: correctly identify binary name
Let me know if you would like a selection mechanism and I'll see what I can do.
I'm confused. It sounds like Heinrich is using sandbox to test on various hosts, how U-Boot behaves, without relying on "just test in QEMU". I don't see how this disconnects from your usual request on how to use sandbox for further testing. What is breaking for you, Simon, by us saying that the host instruction set determines what we load and run (and so, is portable whereas saying "sandbox" is not, between x86 hosts, arm64 hosts and riscv64 hosts).
This:
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 idea of using sandbox to run native EFI apps is new to me, but it sounds great, if it will get us more tests in this area.
But sandbox is sandbox. It is not (x86, arm, riscv). It is a fake architecture that we use in U-Boot. I've offered to create a way to get the behaviour that Heinrich wants, so let's see what he says.
Regards, Simon
Am 18. Oktober 2024 05:05:08 MESZ schrieb Simon Glass sjg@chromium.org:
Hi Tom,
On Thu, 17 Oct 2024 at 18:17, Tom Rini trini@konsulko.com wrote:
On Thu, Oct 17, 2024 at 05:23:19PM -0600, Simon Glass wrote:
Hi Heinrich,
On Mon, 30 Sept 2024 at 17:23, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 26.09.24 23:59, 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.
Drop the confusing use of host architecture and just let sandbox be sandbox.
As I already wrote in https://lore.kernel.org/u-boot/ae1cf1fa-766e-46a0-8ef9-2c2c7af73d9e@gmx.de/ this patch should not be merged.
bootsbx.efi does not exist in the UEFI specification.
If that is of concern I can get it added. Let me know.
Without this patch I can test that shim work and grub are correctly loaded from a distro image. This patch makes the sandbox misbehave.
Why don't you do that with QEMU?
If we want sandbox to do this, I could add a way for sandbox to select its architecture. But this patch is correct, sorry. It is basically a revert of your patch:
3a0654ecd0d efi_loader: correctly identify binary name
Let me know if you would like a selection mechanism and I'll see what I can do.
I'm confused. It sounds like Heinrich is using sandbox to test on various hosts, how U-Boot behaves, without relying on "just test in QEMU". I don't see how this disconnects from your usual request on how to use sandbox for further testing. What is breaking for you, Simon, by us saying that the host instruction set determines what we load and run (and so, is portable whereas saying "sandbox" is not, between x86 hosts, arm64 hosts and riscv64 hosts).
This:
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.
As I wrote before:
The UEFI specification prescribes the naming of the default boot binary. Deviating from the specification should be avoided.
Anyway you need different images per host architecture. Writing a single switch statement to derive the default EFI binary name from the OS architecture does not look like an undue burden.
The sandbox should remain able to start GRUB from a distro image for the host architecture.
The sandbox is not a fake architecture but can be considered a valid testbed for host native EFI binaries including GRUB, the kernel stub, the stubs of UKI images, and the EFI SCT.
Best regards
Heinrich
The idea of using sandbox to run native EFI apps is new to me, but it sounds great, if it will get us more tests in this area.
But sandbox is sandbox. It is not (x86, arm, riscv). It is a fake architecture that we use in U-Boot. I've offered to create a way to get the behaviour that Heinrich wants, so let's see what he says.
Regards, Simon
Hi Heinrich,
On Thu, 17 Oct 2024 at 21:45, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 18. Oktober 2024 05:05:08 MESZ schrieb Simon Glass sjg@chromium.org:
Hi Tom,
On Thu, 17 Oct 2024 at 18:17, Tom Rini trini@konsulko.com wrote:
On Thu, Oct 17, 2024 at 05:23:19PM -0600, Simon Glass wrote:
Hi Heinrich,
On Mon, 30 Sept 2024 at 17:23, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 26.09.24 23:59, 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.
Drop the confusing use of host architecture and just let sandbox be sandbox.
As I already wrote in https://lore.kernel.org/u-boot/ae1cf1fa-766e-46a0-8ef9-2c2c7af73d9e@gmx.de/ this patch should not be merged.
bootsbx.efi does not exist in the UEFI specification.
If that is of concern I can get it added. Let me know.
Without this patch I can test that shim work and grub are correctly loaded from a distro image. This patch makes the sandbox misbehave.
Why don't you do that with QEMU?
If we want sandbox to do this, I could add a way for sandbox to select its architecture. But this patch is correct, sorry. It is basically a revert of your patch:
3a0654ecd0d efi_loader: correctly identify binary name
Let me know if you would like a selection mechanism and I'll see what I can do.
I'm confused. It sounds like Heinrich is using sandbox to test on various hosts, how U-Boot behaves, without relying on "just test in QEMU". I don't see how this disconnects from your usual request on how to use sandbox for further testing. What is breaking for you, Simon, by us saying that the host instruction set determines what we load and run (and so, is portable whereas saying "sandbox" is not, between x86 hosts, arm64 hosts and riscv64 hosts).
This:
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.
As I wrote before:
The UEFI specification prescribes the naming of the default boot binary. Deviating from the specification should be avoided.
As I said before, I'm happy to get this added to the spec, if it helps. It is a real use case.
Anyway you need different images per host architecture. Writing a single switch statement to derive the default EFI binary name from the OS architecture does not look like an undue burden.
Yes, it is easy, but it is taking sandbox in a strange direction. It becomes a hybrid of a host architecture and a U-Boot architecture, which is then going to lead to all sorts of strange conversations. At present it is pretty clear. So I want to head this off at the pass.
I did not get a chance to review the original patch, of which this is essentially a revert. It has actually bitten me in a few areas.
The sandbox should remain able to start GRUB from a distro image for the host architecture.
The sandbox is not a fake architecture but can be considered a valid testbed for host native EFI binaries including GRUB, the kernel stub, the stubs of UKI images, and the EFI SCT.
I have to say that I wrote sandbox and I know what it is for. It wasn't designed to run binaries built outside of U-Boot, or anything of the sort. That said, as I mentioned, I should be able to make something work here.
To help me come up with a solution, can you please send your workflow / commands for the testing you do? Also, what architectures do you do this with?
My approach to U-Boot is that code needs to land, one way or another, perhaps with some changes for the use case that is thrown up, or perhaps done another way. But please try to consider this approach to Open Source too.
Regards, Simon
Best regards
Heinrich
The idea of using sandbox to run native EFI apps is new to me, but it sounds great, if it will get us more tests in this area.
But sandbox is sandbox. It is not (x86, arm, riscv). It is a fake architecture that we use in U-Boot. I've offered to create a way to get the behaviour that Heinrich wants, so let's see what he says.
Regards, SImon
Am 18. Oktober 2024 17:02:47 MESZ schrieb Simon Glass sjg@chromium.org:
Hi Heinrich,
On Thu, 17 Oct 2024 at 21:45, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 18. Oktober 2024 05:05:08 MESZ schrieb Simon Glass sjg@chromium.org:
Hi Tom,
On Thu, 17 Oct 2024 at 18:17, Tom Rini trini@konsulko.com wrote:
On Thu, Oct 17, 2024 at 05:23:19PM -0600, Simon Glass wrote:
Hi Heinrich,
On Mon, 30 Sept 2024 at 17:23, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 26.09.24 23:59, 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. > > Drop the confusing use of host architecture and just let sandbox be > sandbox.
As I already wrote in https://lore.kernel.org/u-boot/ae1cf1fa-766e-46a0-8ef9-2c2c7af73d9e@gmx.de/ this patch should not be merged.
bootsbx.efi does not exist in the UEFI specification.
If that is of concern I can get it added. Let me know.
Without this patch I can test that shim work and grub are correctly loaded from a distro image. This patch makes the sandbox misbehave.
Why don't you do that with QEMU?
If we want sandbox to do this, I could add a way for sandbox to select its architecture. But this patch is correct, sorry. It is basically a revert of your patch:
3a0654ecd0d efi_loader: correctly identify binary name
Let me know if you would like a selection mechanism and I'll see what I can do.
I'm confused. It sounds like Heinrich is using sandbox to test on various hosts, how U-Boot behaves, without relying on "just test in QEMU". I don't see how this disconnects from your usual request on how to use sandbox for further testing. What is breaking for you, Simon, by us saying that the host instruction set determines what we load and run (and so, is portable whereas saying "sandbox" is not, between x86 hosts, arm64 hosts and riscv64 hosts).
This:
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.
As I wrote before:
The UEFI specification prescribes the naming of the default boot binary. Deviating from the specification should be avoided.
As I said before, I'm happy to get this added to the spec, if it helps. It is a real use case.
Do you really think that the UEFI organization should care about your test preference? This is just absurd.
As tests should run on all devices including but not restricted to the sandbox we have to support different names for the default file anyway.
This patch has no virtue in this context.
Best regards
Heinrich
Anyway you need different images per host architecture. Writing a single switch statement to derive the default EFI binary name from the OS architecture does not look like an undue burden.
Yes, it is easy, but it is taking sandbox in a strange direction. It becomes a hybrid of a host architecture and a U-Boot architecture, which is then going to lead to all sorts of strange conversations. At present it is pretty clear. So I want to head this off at the pass.
I did not get a chance to review the original patch, of which this is essentially a revert. It has actually bitten me in a few areas.
The sandbox should remain able to start GRUB from a distro image for the host architecture.
The sandbox is not a fake architecture but can be considered a valid testbed for host native EFI binaries including GRUB, the kernel stub, the stubs of UKI images, and the EFI SCT.
I have to say that I wrote sandbox and I know what it is for. It wasn't designed to run binaries built outside of U-Boot, or anything of the sort. That said, as I mentioned, I should be able to make something work here.
To help me come up with a solution, can you please send your workflow / commands for the testing you do? Also, what architectures do you do this with?
My approach to U-Boot is that code needs to land, one way or another, perhaps with some changes for the use case that is thrown up, or perhaps done another way. But please try to consider this approach to Open Source too.
Regards, Simon
Best regards
Heinrich
The idea of using sandbox to run native EFI apps is new to me, but it sounds great, if it will get us more tests in this area.
But sandbox is sandbox. It is not (x86, arm, riscv). It is a fake architecture that we use in U-Boot. I've offered to create a way to get the behaviour that Heinrich wants, so let's see what he says.
Regards, SImon
Add a little debugging so we can see what is happening.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org ---
Changes in v6: - Expand the debug messages to be more descriptive
boot/bootmeth_efi.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c index 6b41c0999f1..2ad6d3b4ace 100644 --- a/boot/bootmeth_efi.c +++ b/boot/bootmeth_efi.c @@ -162,8 +162,10 @@ static int distro_efi_try_bootflow_files(struct udevice *dev, int ret, seq;
/* We require a partition table */ - if (!bflow->part) + if (!bflow->part) { + log_debug("no partitions\n"); return -ENOENT; + }
strcpy(fname, EFI_DIRNAME); strcat(fname, BOOTEFI_NAME); @@ -171,8 +173,10 @@ static int distro_efi_try_bootflow_files(struct udevice *dev, if (bflow->blk) desc = dev_get_uclass_plat(bflow->blk); ret = bootmeth_try_file(bflow, desc, NULL, fname); - if (ret) + if (ret) { + log_debug("File '%s' not found\n", fname); return log_msg_ret("try", ret); + }
/* Since we can access the file, let's call it ready */ bflow->state = BOOTFLOWST_READY; @@ -307,6 +311,8 @@ static int distro_efi_read_bootflow(struct udevice *dev, struct bootflow *bflow) { int ret;
+ log_debug("dev='%s', part=%d\n", bflow->dev->name, bflow->part); + /* * bootmeth_efi doesn't allocate any buffer neither for blk nor net device * set flag to avoid freeing static buffer. @@ -332,6 +338,7 @@ static int distro_efi_boot(struct udevice *dev, struct bootflow *bflow) ulong kernel, fdt; int ret;
+ log_debug("distro EFI boot\n"); kernel = env_get_hex("kernel_addr_r", 0); if (!bootmeth_uses_network(bflow)) { ret = efiload_read_file(bflow, kernel);
We don't want ANSI characters written in tests since it is a pain to check the output with ut_assert_nextline() et al.
Provide a way to tests to request that ANSI characters not be sent.
Add a proper function comment while we are here, to encourage others.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
include/efi_loader.h | 21 ++++++++++++++++++++- lib/efi_loader/efi_console.c | 26 +++++++++++++++++--------- 2 files changed, 37 insertions(+), 10 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index f84852e384f..82b90ee0f1d 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -531,8 +531,27 @@ efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index); efi_status_t efi_bootmgr_run(void *fdt); /* search the boot option index in BootOrder */ bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *index); -/* Set up console modes */ + +/** + * efi_setup_console_size() - update the mode table. + * + * By default the only mode available is 80x25. If the console has at least 50 + * lines, enable mode 80x50. If we can query the console size and it is neither + * 80x25 nor 80x50, set it as an additional mode. + */ void efi_setup_console_size(void); + +/** + * efi_console_set_ansi() - Set whether ANSI characters should be emitted + * + * These characters mess up tests which use ut_assert_nextline(). Call this + * function to tell efi_loader not to emit these characters when starting up the + * terminal + * + * @allow_ansi: Allow emitting ANSI characters + */ +void efi_console_set_ansi(bool allow_ansi); + /* Set up load options from environment variable */ efi_status_t efi_env_set_load_options(efi_handle_t handle, const char *env_var, u16 **load_options); diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index cea50c748aa..569fc9199bc 100644 --- a/lib/efi_loader/efi_console.c +++ b/lib/efi_loader/efi_console.c @@ -30,6 +30,17 @@ struct cout_mode {
__maybe_unused static struct efi_object uart_obj;
+/* + * suppress emission of ANSI codes for use by unit tests. Leave it as 0 for the + * default behaviour + */ +static bool no_ansi; + +void efi_console_set_ansi(bool allow_ansi) +{ + no_ansi = !allow_ansi; +} + static struct cout_mode efi_cout_modes[] = { /* EFI Mode 0 is 80x25 and always present */ { @@ -348,13 +359,6 @@ static int __maybe_unused query_vidconsole(int *rows, int *cols) return 0; }
-/** - * efi_setup_console_size() - update the mode table. - * - * By default the only mode available is 80x25. If the console has at least 50 - * lines, enable mode 80x50. If we can query the console size and it is neither - * 80x25 nor 80x50, set it as an additional mode. - */ void efi_setup_console_size(void) { int rows = 25, cols = 80; @@ -362,8 +366,12 @@ void efi_setup_console_size(void)
if (IS_ENABLED(CONFIG_VIDEO)) ret = query_vidconsole(&rows, &cols); - if (ret) - ret = query_console_serial(&rows, &cols); + if (ret) { + if (no_ansi) + ret = 0; + else + ret = query_console_serial(&rows, &cols); + } if (ret) return;
On 26.09.24 23:59, 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
Please, consider prior review before resubmitting patches.
As responded to all prior submissions:
We want to test the code running on actual machines. We don't want to have sandbox code everywhere.
I cannot see any test that is not passing due to the current behavior.
The size serial console is just requested once in the live-time of the U-Boot session.
You have bunches of options in your upcoming tests. Neither of these requires the suggested patch:
* You can set stdout=vidconsole,serial and let the vidconsole provide a screen size. * You can add the ANSI sequence to your ut_assert_nextline() statement. * You can filter out ANSI codes in the test framework. * You can run an EFI command like 'printenv -e' before the command that you actually want to test.
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;elseret = query_console_serial(&rows, &cols);- } if (ret) return;
On Tue, Oct 01, 2024 at 01:38:56AM +0200, Heinrich Schuchardt wrote:
On 26.09.24 23:59, 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
Please, consider prior review before resubmitting patches.
As responded to all prior submissions:
We want to test the code running on actual machines. We don't want to have sandbox code everywhere.
I cannot see any test that is not passing due to the current behavior.
The pytests for the EFI selftests are unreliable for me, on Raspberry Pi 3, more often in 32bit mode than 64bit mode, but I feel like I see it there too. And when they fail, the console log is full of ANSI escape sequences. Is this specific test a test you run regularly on real hardware?
On 10/1/24 02:24, Tom Rini wrote:
On Tue, Oct 01, 2024 at 01:38:56AM +0200, Heinrich Schuchardt wrote:
On 26.09.24 23:59, 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
Please, consider prior review before resubmitting patches.
As responded to all prior submissions:
We want to test the code running on actual machines. We don't want to have sandbox code everywhere.
I cannot see any test that is not passing due to the current behavior.
The pytests for the EFI selftests are unreliable for me, on Raspberry Pi 3, more often in 32bit mode than 64bit mode, but I feel like I see it there too. And when they fail, the console log is full of ANSI escape sequences. Is this specific test a test you run regularly on real hardware?
It is not only the EFI test but also pytest adding color to the console output.
When I download a raw LOG and display it with
wget https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/904222/raw less -r raw
I see the correct colored output. Same when I run
cat raw
If you want to strip ANSI codes from a file, you can use ansi2txt from Ubuntu package colorized logs.
ansi2txt < raw > flat.log
I typically run UEFI tests interactively on the sandbox, virtual machines, and on actual hardware.
lib/efi_selftest/efi_selftest_textoutput.c is specifically used to print out all combinations of fore- and background colors.
Furthermore color is used in /lib/efi_selftest/ for:
* start of test * success, warning, error
This coloring is not functionally necessary but for my taste makes the output easier to read.
test/py/tests/test_efi_selftest.py just has no problems with the color output when running:
if u_boot_console.p.expect(['Summary: 0 failures', 'Press any key']):
While color output in UEFI unit tests as said is not functionally necessary in most tests, the current patch tries to suppress ANSI output which is functionally necessary and which probably should be tested for.
Best regards
Heinrich
On Tue, Oct 01, 2024 at 04:34:54AM +0200, Heinrich Schuchardt wrote:
On 10/1/24 02:24, Tom Rini wrote:
On Tue, Oct 01, 2024 at 01:38:56AM +0200, Heinrich Schuchardt wrote:
On 26.09.24 23:59, 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
Please, consider prior review before resubmitting patches.
As responded to all prior submissions:
We want to test the code running on actual machines. We don't want to have sandbox code everywhere.
I cannot see any test that is not passing due to the current behavior.
The pytests for the EFI selftests are unreliable for me, on Raspberry Pi 3, more often in 32bit mode than 64bit mode, but I feel like I see it there too. And when they fail, the console log is full of ANSI escape sequences. Is this specific test a test you run regularly on real hardware?
It is not only the EFI test but also pytest adding color to the console output.
Alright, so at least for the problems I have _today_, I've figured it out, and the problem is that the watchdog test fails too quickly:
[snip] test/py/tests/test_efi_selftest.py ..F
=================================== FAILURES =================================== ______________________ test_efi_selftest_watchdog_reboot _______________________ test/py/tests/test_efi_selftest.py:61: in test_efi_selftest_watchdog_reboot u_boot_console.restart_uboot() test/py/u_boot_console_base.py:478: in restart_uboot self.ensure_spawned(expect_reset) test/py/u_boot_console_base.py:442: in ensure_spawned self.wait_for_boot_prompt(loop_num = loop_num) test/py/u_boot_console_base.py:195: in wait_for_boot_prompt raise Exception('Bad pattern found on console: ' + E Exception: Bad pattern found on console: spl_signon ----------------------------- Captured stdout call ----------------------------- => setenv efi_selftest list => => bootefi selftest 7[r[999;999H[6n8No EFI system partition No EFI system partition Failed to persist EFI variables No EFI system partition Failed to persist EFI variables No EFI system partition Failed to persist EFI variables
Available tests: 'block image transfer' - on request 'block device' 'configuration tables' 'controllers' 'crc32' 'device path' 'device path utilities protocol' 'conformance profile table' 'event groups' 'event services' 'exception' - on request 'ExitBootServices' 'device tree' 'graphical output' 'HII database protocols' 'load file protocol' 'loaded image' 'load image from file' 'mem' 'memory' 'open protocol' 'manage protocols' 'register protocol notify' 'reset system' - on request 'reset system runtime' - on request 'real time clock' 'simple network protocol' 'start image return' 'start image exit' 'text input' - on request 'extended text input' - on request 'text output' 'task priority levels' 'unicode collation' 'variables' 'variables at runtime' 'virtual address map' 'watchdog timer' 'watchdog reboot' - on request => => setenv efi_selftest watchdog reboot => => bootefi selftest [1;37;40m Testing EFI API implementation [0;37;40m[1;37;40m Selected test: 'watchdog reboot' [0;37;40m[1;34;40m Setting up 'watchdog reboot' [0;37;40m[1;32;40mSetting up 'watchdog reboot' succeeded [0;37;40m[1;34;40m Executing 'watchdog reboot' [0;37;40m EFI: Watchdog timeout resetting ... +u-boot-test-reset am64x_evm_a53 na Selected role am64-sk from configuration file Selected role am64-sk from configuration file connecting to NetworkSerialPort(target=Target(name='am64-sk', env=Environment(config_file='/home/trini/u-boot/lg_env.yaml')), name='USBSerialPort', state=<BindingState.bound: 1>, avail=True, host='ti-lab-host', port=57479, speed=115200, protocol='rfc2217') calling microcom -s 115200 -t ti-lab-host:57479 connected to 192.168.116.10 (port 57479) Escape character: Ctrl-\ Type the escape character to get to the prompt.
U-Boot SPL 2024.10-rc5-00022-g17da9795c115 (Oct 01 2024 - 14:29:10 +0000) SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)') EEPROM not available at 0x50, trying to read at 0x51 SPL initial stack usage: 13368 bytes Trying to boot from MMC2 Loading Environment from MMC... MMC Device 0 not found *** Warning - No MMC card found, using default environment
Starting ATF on ARM64 core...
NOTICE: BL31: v2.10.0(release):v2.10.0-729-gc8be7c08c NOTICE: BL31: Built : 13:50:07, Apr 24 2024 I/TC: I/TC: OP-TEE version: 4.2.0-22-g16fbd46d2 (gcc version 13.2.0 (GCC)) #2 Wed Apr 24 19:50:23 UTC 2024 aarch64 I/TC: WARNING: This OP-TEE configuration might be insecure! I/TC: WARNING: Please check https://optee.readthedocs.io/en/latest/architecture/porting_guidelines.html I/TC: Primary CPU initializing I/TC: GIC redistributor base address not provided I/TC: Assuming default GIC group status and modifier I/TC: SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)') I/TC: HUK Initialized I/TC: Activated SA2UL device I/TC: Fixing SA2UL firewall owner for GP device I/TC: Enabled firewalls for SA2UL TRNG device I/TC: SA2UL TRNG initialized I/TC: SA2UL Drivers initialized I/TC: Primary CPU switching to normal world boot
U-Boot SPL 2024.10-rc5-00022-g17da9795c115 (Oct 01 2024 - 14:31:31 +0000) SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)') Trying to boot from MMC2 ? U-Boot SPL 2024.10-rc5-00022-g17da9795c115 (Oct 01 2024 - 14:29:10 +0000) Resetting on cold boot to workaround ErrataID:i2331 Please resend tiboot3.bin in case of UART/DFU boot resetting ...
U-Boot SPL 2024.10-rc5-00022-g17da9795c115 (Oct 01 2024 - 14:29:10 +0000) =========================== short test summary info ============================ SKIPPED [47] test/py/conftest.py:512: board "am64x_evm_a53" not supported SKIPPED [5] test/py/conftest.py:531: .config feature "cmd_avb" not enabled SKIPPED [1] test/py/conftest.py:531: .config feature "optee_ta_avb" not enabled SKIPPED [1] test/py/conftest.py:531: .config feature "cmd_bootstage" not enabled SKIPPED [2] test/py/conftest.py:531: .config feature "bootstage_stash" not enabled SKIPPED [1] test/py/tests/test_dfu.py:114: got empty parameter set ['env__usb_dev_port'], function test_dfu at /home/trini/u-boot/u-boot/test/py/tests/test_dfu.py:113 SKIPPED [1] test/py/tests/test_efi_fit.py:401: No env__efi_fit_tftp_file binary specified in environment FAILED test/py/tests/test_efi_selftest.py::test_efi_selftest_watchdog_reboot !!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!! ================== 1 failed, 17 passed, 58 skipped in 58.92s ===================
I thought it was the escape sequences confusing the check rather than the platform takes longer than expected to provide whatever the expected string is. Another TI K3 platform gets slightly farther along before failing the same way. I'm going back to looping over Pis to see if they fail in a way that they used to in my previous lab setup.
On 10/1/24 20:02, Tom Rini wrote:
On Tue, Oct 01, 2024 at 04:34:54AM +0200, Heinrich Schuchardt wrote:
On 10/1/24 02:24, Tom Rini wrote:
On Tue, Oct 01, 2024 at 01:38:56AM +0200, Heinrich Schuchardt wrote:
On 26.09.24 23:59, 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
Please, consider prior review before resubmitting patches.
As responded to all prior submissions:
We want to test the code running on actual machines. We don't want to have sandbox code everywhere.
I cannot see any test that is not passing due to the current behavior.
The pytests for the EFI selftests are unreliable for me, on Raspberry Pi 3, more often in 32bit mode than 64bit mode, but I feel like I see it there too. And when they fail, the console log is full of ANSI escape sequences. Is this specific test a test you run regularly on real hardware?
It is not only the EFI test but also pytest adding color to the console output.
Alright, so at least for the problems I have _today_, I've figured it out, and the problem is that the watchdog test fails too quickly:
[snip] test/py/tests/test_efi_selftest.py ..F test/py/u_boot_console_base.p =================================== FAILURES =================================== ______________________ test_efi_selftest_watchdog_reboot _______________________ test/py/tests/test_efi_selftest.py:61: in test_efi_selftest_watchdog_reboot u_boot_console.restart_uboot() test/py/u_boot_console_base.py:478: in restart_uboot self.ensure_spawned(expect_reset) test/py/u_boot_console_base.py:442: in ensure_spawned self.wait_for_boot_prompt(loop_num = loop_num) test/py/u_boot_console_base.py:195: in wait_for_boot_prompt raise Exception('Bad pattern found on console: ' + E Exception: Bad pattern found on console: spl_signon ----------------------------- Captured stdout call ----------------------------- => setenv efi_selftest list => => bootefi selftest 7[r[999;999H[6n8No EFI system partition No EFI system partition Failed to persist EFI variables No EFI system partition Failed to persist EFI variables No EFI system partition Failed to persist EFI variables
Available tests: 'block image transfer' - on request 'block device' 'configuration tables' 'controllers' 'crc32' 'device path' 'device path utilities protocol' 'conformance profile table' 'event groups' 'event services' 'exception' - on request 'ExitBootServices' 'device tree' 'graphical output' 'HII database protocols' 'load file protocol' 'loaded image' 'load image from file' 'mem' 'memory' 'open protocol' 'manage protocols' 'register protocol notify' 'reset system' - on request 'reset system runtime' - on request 'real time clock' 'simple network protocol' 'start image return' 'start image exit' 'text input' - on request 'extended text input' - on request 'text output' 'task priority levels' 'unicode collation' 'variables' 'variables at runtime' 'virtual address map' 'watchdog timer' 'watchdog reboot' - on request => => setenv efi_selftest watchdog reboot => => bootefi selftest [1;37;40m Testing EFI API implementation [0;37;40m[1;37;40m Selected test: 'watchdog reboot' [0;37;40m[1;34;40m Setting up 'watchdog reboot' [0;37;40m[1;32;40mSetting up 'watchdog reboot' succeeded [0;37;40m[1;34;40m Executing 'watchdog reboot' [0;37;40m EFI: Watchdog timeout resetting ... +u-boot-test-reset am64x_evm_a53 na Selected role am64-sk from configuration file Selected role am64-sk from configuration file connecting to NetworkSerialPort(target=Target(name='am64-sk', env=Environment(config_file='/home/trini/u-boot/lg_env.yaml')), name='USBSerialPort', state=<BindingState.bound: 1>, avail=True, host='ti-lab-host', port=57479, speed=115200, protocol='rfc2217') calling microcom -s 115200 -t ti-lab-host:57479 connected to 192.168.116.10 (port 57479) Escape character: Ctrl-\ Type the escape character to get to the prompt.
U-Boot SPL 2024.10-rc5-00022-g17da9795c115 (Oct 01 2024 - 14:29:10 +0000) SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)') EEPROM not available at 0x50, trying to read at 0x51 SPL initial stack usage: 13368 bytes Trying to boot from MMC2 Loading Environment from MMC... MMC Device 0 not found *** Warning - No MMC card found, using default environment
Starting ATF on ARM64 core...
NOTICE: BL31: v2.10.0(release):v2.10.0-729-gc8be7c08c NOTICE: BL31: Built : 13:50:07, Apr 24 2024 I/TC: I/TC: OP-TEE version: 4.2.0-22-g16fbd46d2 (gcc version 13.2.0 (GCC)) #2 Wed Apr 24 19:50:23 UTC 2024 aarch64 I/TC: WARNING: This OP-TEE configuration might be insecure! I/TC: WARNING: Please check https://optee.readthedocs.io/en/latest/architecture/porting_guidelines.html I/TC: Primary CPU initializing I/TC: GIC redistributor base address not provided I/TC: Assuming default GIC group status and modifier I/TC: SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)') I/TC: HUK Initialized I/TC: Activated SA2UL device I/TC: Fixing SA2UL firewall owner for GP device I/TC: Enabled firewalls for SA2UL TRNG device I/TC: SA2UL TRNG initialized I/TC: SA2UL Drivers initialized I/TC: Primary CPU switching to normal world boot
U-Boot SPL 2024.10-rc5-00022-g17da9795c115 (Oct 01 2024 - 14:31:31 +0000) SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)') Trying to boot from MMC2 ? U-Boot SPL 2024.10-rc5-00022-g17da9795c115 (Oct 01 2024 - 14:29:10 +0000) Resetting on cold boot to workaround ErrataID:i2331 Please resend tiboot3.bin in case of UART/DFU boot resetting ...
U-Boot SPL 2024.10-rc5-00022-g17da9795c115 (Oct 01 2024 - 14:29:10 +0000) =========================== short test summary info ============================ SKIPPED [47] test/py/conftest.py:512: board "am64x_evm_a53" not supported SKIPPED [5] test/py/conftest.py:531: .config feature "cmd_avb" not enabled SKIPPED [1] test/py/conftest.py:531: .config feature "optee_ta_avb" not enabled SKIPPED [1] test/py/conftest.py:531: .config feature "cmd_bootstage" not enabled SKIPPED [2] test/py/conftest.py:531: .config feature "bootstage_stash" not enabled SKIPPED [1] test/py/tests/test_dfu.py:114: got empty parameter set ['env__usb_dev_port'], function test_dfu at /home/trini/u-boot/u-boot/test/py/tests/test_dfu.py:113 SKIPPED [1] test/py/tests/test_efi_fit.py:401: No env__efi_fit_tftp_file binary specified in environment FAILED test/py/tests/test_efi_selftest.py::test_efi_selftest_watchdog_reboot !!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!! ================== 1 failed, 17 passed, 58 skipped in 58.92s ===================
I thought it was the escape sequences confusing the check rather than the platform takes longer than expected to provide whatever the expected string is. Another TI K3 platform gets slightly farther along before failing the same way. I'm going back to looping over Pis to see if they fail in a way that they used to in my previous lab setup.
The relevant error message is:
E Exception: Bad pattern found on console: spl_signon
The error occurs because the substring
U-Boot SPL 2024.10
occurs multiple times during the reboot process. This is not expected in wait_for_boot_prompt().
The EFI test is resetting the board and then calling restart_uboot() which by itself does another reset.
While the watchdog triggered reboot is running thee test environment may need some time before triggering a reboot itself. This may lead to the observed duplicate output of the U-Boot greeter which is recorded as an error.
Maybe we should directly call wait_for_boot_prompt(loop_num = 2).
diff --git a/test/py/tests/test_efi_selftest.py b/test/py/tests/test_efi_selftest.py index 43f24245582..c3ee1888afb 100644 --- a/test/py/tests/test_efi_selftest.py +++ b/test/py/tests/test_efi_selftest.py @@ -58,7 +58,7 @@ def test_efi_selftest_watchdog_reboot(u_boot_console): u_boot_console.run_command(cmd='bootefi selftest', wait_for_prompt=False) if u_boot_console.p.expect(['resetting', 'U-Boot']): raise Exception('Reset failed in 'watchdog reboot' test') - u_boot_console.restart_uboot() + u_boot_console.wait_for_boot_prompt(loop_num = 2)
@pytest.mark.buildconfigspec('cmd_bootefi_selftest') def test_efi_selftest_text_input(u_boot_console):
Thanks for bringing the issue to my attention.
Best regards
Heinrich
On Wed, Oct 02, 2024 at 12:18:39AM +0200, Heinrich Schuchardt wrote:
On 10/1/24 20:02, Tom Rini wrote:
On Tue, Oct 01, 2024 at 04:34:54AM +0200, Heinrich Schuchardt wrote:
On 10/1/24 02:24, Tom Rini wrote:
On Tue, Oct 01, 2024 at 01:38:56AM +0200, Heinrich Schuchardt wrote:
On 26.09.24 23:59, 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
Please, consider prior review before resubmitting patches.
As responded to all prior submissions:
We want to test the code running on actual machines. We don't want to have sandbox code everywhere.
I cannot see any test that is not passing due to the current behavior.
The pytests for the EFI selftests are unreliable for me, on Raspberry Pi 3, more often in 32bit mode than 64bit mode, but I feel like I see it there too. And when they fail, the console log is full of ANSI escape sequences. Is this specific test a test you run regularly on real hardware?
It is not only the EFI test but also pytest adding color to the console output.
Alright, so at least for the problems I have _today_, I've figured it out, and the problem is that the watchdog test fails too quickly:
[snip] test/py/tests/test_efi_selftest.py ..F test/py/u_boot_console_base.p =================================== FAILURES =================================== ______________________ test_efi_selftest_watchdog_reboot _______________________ test/py/tests/test_efi_selftest.py:61: in test_efi_selftest_watchdog_reboot u_boot_console.restart_uboot() test/py/u_boot_console_base.py:478: in restart_uboot self.ensure_spawned(expect_reset) test/py/u_boot_console_base.py:442: in ensure_spawned self.wait_for_boot_prompt(loop_num = loop_num) test/py/u_boot_console_base.py:195: in wait_for_boot_prompt raise Exception('Bad pattern found on console: ' + E Exception: Bad pattern found on console: spl_signon ----------------------------- Captured stdout call ----------------------------- => setenv efi_selftest list => => bootefi selftest 7[r[999;999H[6n8No EFI system partition No EFI system partition Failed to persist EFI variables No EFI system partition Failed to persist EFI variables No EFI system partition Failed to persist EFI variables
Available tests: 'block image transfer' - on request 'block device' 'configuration tables' 'controllers' 'crc32' 'device path' 'device path utilities protocol' 'conformance profile table' 'event groups' 'event services' 'exception' - on request 'ExitBootServices' 'device tree' 'graphical output' 'HII database protocols' 'load file protocol' 'loaded image' 'load image from file' 'mem' 'memory' 'open protocol' 'manage protocols' 'register protocol notify' 'reset system' - on request 'reset system runtime' - on request 'real time clock' 'simple network protocol' 'start image return' 'start image exit' 'text input' - on request 'extended text input' - on request 'text output' 'task priority levels' 'unicode collation' 'variables' 'variables at runtime' 'virtual address map' 'watchdog timer' 'watchdog reboot' - on request => => setenv efi_selftest watchdog reboot => => bootefi selftest [1;37;40m Testing EFI API implementation [0;37;40m[1;37;40m Selected test: 'watchdog reboot' [0;37;40m[1;34;40m Setting up 'watchdog reboot' [0;37;40m[1;32;40mSetting up 'watchdog reboot' succeeded [0;37;40m[1;34;40m Executing 'watchdog reboot' [0;37;40m EFI: Watchdog timeout resetting ... +u-boot-test-reset am64x_evm_a53 na Selected role am64-sk from configuration file Selected role am64-sk from configuration file connecting to NetworkSerialPort(target=Target(name='am64-sk', env=Environment(config_file='/home/trini/u-boot/lg_env.yaml')), name='USBSerialPort', state=<BindingState.bound: 1>, avail=True, host='ti-lab-host', port=57479, speed=115200, protocol='rfc2217') calling microcom -s 115200 -t ti-lab-host:57479 connected to 192.168.116.10 (port 57479) Escape character: Ctrl-\ Type the escape character to get to the prompt.
U-Boot SPL 2024.10-rc5-00022-g17da9795c115 (Oct 01 2024 - 14:29:10 +0000) SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)') EEPROM not available at 0x50, trying to read at 0x51 SPL initial stack usage: 13368 bytes Trying to boot from MMC2 Loading Environment from MMC... MMC Device 0 not found *** Warning - No MMC card found, using default environment
Starting ATF on ARM64 core...
NOTICE: BL31: v2.10.0(release):v2.10.0-729-gc8be7c08c NOTICE: BL31: Built : 13:50:07, Apr 24 2024 I/TC: I/TC: OP-TEE version: 4.2.0-22-g16fbd46d2 (gcc version 13.2.0 (GCC)) #2 Wed Apr 24 19:50:23 UTC 2024 aarch64 I/TC: WARNING: This OP-TEE configuration might be insecure! I/TC: WARNING: Please check https://optee.readthedocs.io/en/latest/architecture/porting_guidelines.html I/TC: Primary CPU initializing I/TC: GIC redistributor base address not provided I/TC: Assuming default GIC group status and modifier I/TC: SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)') I/TC: HUK Initialized I/TC: Activated SA2UL device I/TC: Fixing SA2UL firewall owner for GP device I/TC: Enabled firewalls for SA2UL TRNG device I/TC: SA2UL TRNG initialized I/TC: SA2UL Drivers initialized I/TC: Primary CPU switching to normal world boot
U-Boot SPL 2024.10-rc5-00022-g17da9795c115 (Oct 01 2024 - 14:31:31 +0000) SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)') Trying to boot from MMC2 ? U-Boot SPL 2024.10-rc5-00022-g17da9795c115 (Oct 01 2024 - 14:29:10 +0000) Resetting on cold boot to workaround ErrataID:i2331 Please resend tiboot3.bin in case of UART/DFU boot resetting ...
U-Boot SPL 2024.10-rc5-00022-g17da9795c115 (Oct 01 2024 - 14:29:10 +0000) =========================== short test summary info ============================ SKIPPED [47] test/py/conftest.py:512: board "am64x_evm_a53" not supported SKIPPED [5] test/py/conftest.py:531: .config feature "cmd_avb" not enabled SKIPPED [1] test/py/conftest.py:531: .config feature "optee_ta_avb" not enabled SKIPPED [1] test/py/conftest.py:531: .config feature "cmd_bootstage" not enabled SKIPPED [2] test/py/conftest.py:531: .config feature "bootstage_stash" not enabled SKIPPED [1] test/py/tests/test_dfu.py:114: got empty parameter set ['env__usb_dev_port'], function test_dfu at /home/trini/u-boot/u-boot/test/py/tests/test_dfu.py:113 SKIPPED [1] test/py/tests/test_efi_fit.py:401: No env__efi_fit_tftp_file binary specified in environment FAILED test/py/tests/test_efi_selftest.py::test_efi_selftest_watchdog_reboot !!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!! ================== 1 failed, 17 passed, 58 skipped in 58.92s ===================
I thought it was the escape sequences confusing the check rather than the platform takes longer than expected to provide whatever the expected string is. Another TI K3 platform gets slightly farther along before failing the same way. I'm going back to looping over Pis to see if they fail in a way that they used to in my previous lab setup.
The relevant error message is:
E Exception: Bad pattern found on console: spl_signonThe error occurs because the substring
U-Boot SPL 2024.10occurs multiple times during the reboot process. This is not expected in wait_for_boot_prompt().
The EFI test is resetting the board and then calling restart_uboot() which by itself does another reset.
While the watchdog triggered reboot is running thee test environment may need some time before triggering a reboot itself. This may lead to the observed duplicate output of the U-Boot greeter which is recorded as an error.
Maybe we should directly call wait_for_boot_prompt(loop_num = 2).
diff --git a/test/py/tests/test_efi_selftest.py b/test/py/tests/test_efi_selftest.py index 43f24245582..c3ee1888afb 100644 --- a/test/py/tests/test_efi_selftest.py +++ b/test/py/tests/test_efi_selftest.py @@ -58,7 +58,7 @@ def test_efi_selftest_watchdog_reboot(u_boot_console): u_boot_console.run_command(cmd='bootefi selftest', wait_for_prompt=False) if u_boot_console.p.expect(['resetting', 'U-Boot']): raise Exception('Reset failed in 'watchdog reboot' test')
- u_boot_console.restart_uboot()
- u_boot_console.wait_for_boot_prompt(loop_num = 2)
@pytest.mark.buildconfigspec('cmd_bootefi_selftest') def test_efi_selftest_text_input(u_boot_console):
Thanks for bringing the issue to my attention.
Thanks for digging in to this more. I suspect (and I'll try and confirm soon) that we need to take in to account: commit 645f75f6884a22905b0e3790ca9254fa1a13216e Author: Tom Rini trini@konsulko.com Date: Wed Apr 24 16:45:37 2024 -0600
test/py: Make the number of SPL banners seen a variable
Currently we have the option to tell the console code that we should ignore the SPL banner. We also have an option to say that we can see it a second time, and ignore it. However, some platforms such as TI AM64x will have us see the SPL banner three times. Rather than add an "spl3_skipped" option, rework the code. By default we expect to see the banner once, but boards can specify seeing it as many times as they expect to.
Signed-off-by: Tom Rini trini@konsulko.com
as well in this area.
Best regards
Heinrich
Hi Heinrich, Tom,
On Tue, 1 Oct 2024 at 16:18, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/1/24 20:02, Tom Rini wrote:
On Tue, Oct 01, 2024 at 04:34:54AM +0200, Heinrich Schuchardt wrote:
On 10/1/24 02:24, Tom Rini wrote:
On Tue, Oct 01, 2024 at 01:38:56AM +0200, Heinrich Schuchardt wrote:
On 26.09.24 23:59, 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
Please, consider prior review before resubmitting patches.
As responded to all prior submissions:
We want to test the code running on actual machines. We don't want to have sandbox code everywhere.
I cannot see any test that is not passing due to the current behavior.
The pytests for the EFI selftests are unreliable for me, on Raspberry Pi 3, more often in 32bit mode than 64bit mode, but I feel like I see it there too. And when they fail, the console log is full of ANSI escape sequences. Is this specific test a test you run regularly on real hardware?
It is not only the EFI test but also pytest adding color to the console output.
Alright, so at least for the problems I have _today_, I've figured it out, and the problem is that the watchdog test fails too quickly:
[snip] test/py/tests/test_efi_selftest.py ..F test/py/u_boot_console_base.p =================================== FAILURES =================================== ______________________ test_efi_selftest_watchdog_reboot _______________________ test/py/tests/test_efi_selftest.py:61: in test_efi_selftest_watchdog_reboot u_boot_console.restart_uboot() test/py/u_boot_console_base.py:478: in restart_uboot self.ensure_spawned(expect_reset) test/py/u_boot_console_base.py:442: in ensure_spawned self.wait_for_boot_prompt(loop_num = loop_num) test/py/u_boot_console_base.py:195: in wait_for_boot_prompt raise Exception('Bad pattern found on console: ' + E Exception: Bad pattern found on console: spl_signon ----------------------------- Captured stdout call ----------------------------- => setenv efi_selftest list => => bootefi selftest 7 [r [999;999H [6n 8No EFI system partition No EFI system partition Failed to persist EFI variables No EFI system partition Failed to persist EFI variables No EFI system partition Failed to persist EFI variables
Available tests: 'block image transfer' - on request 'block device' 'configuration tables' 'controllers' 'crc32' 'device path' 'device path utilities protocol' 'conformance profile table' 'event groups' 'event services' 'exception' - on request 'ExitBootServices' 'device tree' 'graphical output' 'HII database protocols' 'load file protocol' 'loaded image' 'load image from file' 'mem' 'memory' 'open protocol' 'manage protocols' 'register protocol notify' 'reset system' - on request 'reset system runtime' - on request 'real time clock' 'simple network protocol' 'start image return' 'start image exit' 'text input' - on request 'extended text input' - on request 'text output' 'task priority levels' 'unicode collation' 'variables' 'variables at runtime' 'virtual address map' 'watchdog timer' 'watchdog reboot' - on request => => setenv efi_selftest watchdog reboot => => bootefi selftest [1;37;40m Testing EFI API implementation [0;37;40m [1;37;40m Selected test: 'watchdog reboot' [0;37;40m [1;34;40m Setting up 'watchdog reboot' [0;37;40m [1;32;40mSetting up 'watchdog reboot' succeeded [0;37;40m [1;34;40m Executing 'watchdog reboot' [0;37;40m EFI: Watchdog timeout resetting ... +u-boot-test-reset am64x_evm_a53 na Selected role am64-sk from configuration file Selected role am64-sk from configuration file connecting to NetworkSerialPort(target=Target(name='am64-sk', env=Environment(config_file='/home/trini/u-boot/lg_env.yaml')), name='USBSerialPort', state=<BindingState.bound: 1>, avail=True, host='ti-lab-host', port=57479, speed=115200, protocol='rfc2217') calling microcom -s 115200 -t ti-lab-host:57479 connected to 192.168.116.10 (port 57479) Escape character: Ctrl-\ Type the escape character to get to the prompt.
U-Boot SPL 2024.10-rc5-00022-g17da9795c115 (Oct 01 2024 - 14:29:10 +0000) SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)') EEPROM not available at 0x50, trying to read at 0x51 SPL initial stack usage: 13368 bytes Trying to boot from MMC2 Loading Environment from MMC... MMC Device 0 not found *** Warning - No MMC card found, using default environment
Starting ATF on ARM64 core...
NOTICE: BL31: v2.10.0(release):v2.10.0-729-gc8be7c08c NOTICE: BL31: Built : 13:50:07, Apr 24 2024 I/TC: I/TC: OP-TEE version: 4.2.0-22-g16fbd46d2 (gcc version 13.2.0 (GCC)) #2 Wed Apr 24 19:50:23 UTC 2024 aarch64 I/TC: WARNING: This OP-TEE configuration might be insecure! I/TC: WARNING: Please check https://optee.readthedocs.io/en/latest/architecture/porting_guidelines.html I/TC: Primary CPU initializing I/TC: GIC redistributor base address not provided I/TC: Assuming default GIC group status and modifier I/TC: SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)') I/TC: HUK Initialized I/TC: Activated SA2UL device I/TC: Fixing SA2UL firewall owner for GP device I/TC: Enabled firewalls for SA2UL TRNG device I/TC: SA2UL TRNG initialized I/TC: SA2UL Drivers initialized I/TC: Primary CPU switching to normal world boot
U-Boot SPL 2024.10-rc5-00022-g17da9795c115 (Oct 01 2024 - 14:31:31 +0000) SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)') Trying to boot from MMC2 ? U-Boot SPL 2024.10-rc5-00022-g17da9795c115 (Oct 01 2024 - 14:29:10 +0000) Resetting on cold boot to workaround ErrataID:i2331 Please resend tiboot3.bin in case of UART/DFU boot resetting ...
U-Boot SPL 2024.10-rc5-00022-g17da9795c115 (Oct 01 2024 - 14:29:10 +0000) =========================== short test summary info ============================ SKIPPED [47] test/py/conftest.py:512: board "am64x_evm_a53" not supported SKIPPED [5] test/py/conftest.py:531: .config feature "cmd_avb" not enabled SKIPPED [1] test/py/conftest.py:531: .config feature "optee_ta_avb" not enabled SKIPPED [1] test/py/conftest.py:531: .config feature "cmd_bootstage" not enabled SKIPPED [2] test/py/conftest.py:531: .config feature "bootstage_stash" not enabled SKIPPED [1] test/py/tests/test_dfu.py:114: got empty parameter set ['env__usb_dev_port'], function test_dfu at /home/trini/u-boot/u-boot/test/py/tests/test_dfu.py:113 SKIPPED [1] test/py/tests/test_efi_fit.py:401: No env__efi_fit_tftp_file binary specified in environment FAILED test/py/tests/test_efi_selftest.py::test_efi_selftest_watchdog_reboot !!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!! ================== 1 failed, 17 passed, 58 skipped in 58.92s ===================
I thought it was the escape sequences confusing the check rather than the platform takes longer than expected to provide whatever the expected string is. Another TI K3 platform gets slightly farther along before failing the same way. I'm going back to looping over Pis to see if they fail in a way that they used to in my previous lab setup.
The relevant error message is:
E Exception: Bad pattern found on console: spl_signonThe error occurs because the substring
U-Boot SPL 2024.10occurs multiple times during the reboot process. This is not expected in wait_for_boot_prompt().
The EFI test is resetting the board and then calling restart_uboot() which by itself does another reset.
We had this discussion a while back and I was very-much opposed to restarting U-Boot within a test. It has set a bad precedent and now several tests do it.
This is another EFI thing that I would like to resolve at some point. As previously mentioned, I understand we might be one test to check that U-Boot does actually kick up a firmware update when started, but I don't think we should be restarting willy-nilly.
While the watchdog triggered reboot is running thee test environment may need some time before triggering a reboot itself. This may lead to the observed duplicate output of the U-Boot greeter which is recorded as an error.
Maybe we should directly call wait_for_boot_prompt(loop_num = 2).
diff --git a/test/py/tests/test_efi_selftest.py b/test/py/tests/test_efi_selftest.py index 43f24245582..c3ee1888afb 100644 --- a/test/py/tests/test_efi_selftest.py +++ b/test/py/tests/test_efi_selftest.py @@ -58,7 +58,7 @@ def test_efi_selftest_watchdog_reboot(u_boot_console): u_boot_console.run_command(cmd='bootefi selftest', wait_for_prompt=False) if u_boot_console.p.expect(['resetting', 'U-Boot']): raise Exception('Reset failed in 'watchdog reboot' test')
- u_boot_console.restart_uboot()
u_boot_console.wait_for_boot_prompt(loop_num = 2)
@pytest.mark.buildconfigspec('cmd_bootefi_selftest') def test_efi_selftest_text_input(u_boot_console):
Thanks for bringing the issue to my attention.
Regards, Simon
On Fri, Oct 11, 2024 at 04:16:28PM -0600, Simon Glass wrote:
Hi Heinrich, Tom,
On Tue, 1 Oct 2024 at 16:18, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
[snip]
The relevant error message is:
E Exception: Bad pattern found on console: spl_signonThe error occurs because the substring
U-Boot SPL 2024.10occurs multiple times during the reboot process. This is not expected in wait_for_boot_prompt().
The EFI test is resetting the board and then calling restart_uboot() which by itself does another reset.
We had this discussion a while back and I was very-much opposed to restarting U-Boot within a test. It has set a bad precedent and now several tests do it.
This is another EFI thing that I would like to resolve at some point. As previously mentioned, I understand we might be one test to check that U-Boot does actually kick up a firmware update when started, but I don't think we should be restarting willy-nilly.
We should indeed not be restarting willy-nilly. And we don't. But the test for "can we restart the system" must restart the system. That's the test. It's not changing. Testing if we can restart the system by faking the restart is not testing restarting the system.
Hi Tom,
On Fri, 11 Oct 2024 at 16:28, Tom Rini trini@konsulko.com wrote:
On Fri, Oct 11, 2024 at 04:16:28PM -0600, Simon Glass wrote:
Hi Heinrich, Tom,
On Tue, 1 Oct 2024 at 16:18, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
[snip]
The relevant error message is:
E Exception: Bad pattern found on console: spl_signonThe error occurs because the substring
U-Boot SPL 2024.10occurs multiple times during the reboot process. This is not expected in wait_for_boot_prompt().
The EFI test is resetting the board and then calling restart_uboot() which by itself does another reset.
We had this discussion a while back and I was very-much opposed to restarting U-Boot within a test. It has set a bad precedent and now several tests do it.
This is another EFI thing that I would like to resolve at some point. As previously mentioned, I understand we might be one test to check that U-Boot does actually kick up a firmware update when started, but I don't think we should be restarting willy-nilly.
We should indeed not be restarting willy-nilly. And we don't. But the test for "can we restart the system" must restart the system. That's the test. It's not changing. Testing if we can restart the system by faking the restart is not testing restarting the system.
If it were just one test I would be OK with it...but it has spread, unfortunately. When I run under gdb I constantly have to manually restart U-Boot.
Regards, SImon
On Sun, Oct 13, 2024 at 01:33:32PM -0600, Simon Glass wrote:
Hi Tom,
On Fri, 11 Oct 2024 at 16:28, Tom Rini trini@konsulko.com wrote:
On Fri, Oct 11, 2024 at 04:16:28PM -0600, Simon Glass wrote:
Hi Heinrich, Tom,
On Tue, 1 Oct 2024 at 16:18, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
[snip]
The relevant error message is:
E Exception: Bad pattern found on console: spl_signonThe error occurs because the substring
U-Boot SPL 2024.10occurs multiple times during the reboot process. This is not expected in wait_for_boot_prompt().
The EFI test is resetting the board and then calling restart_uboot() which by itself does another reset.
We had this discussion a while back and I was very-much opposed to restarting U-Boot within a test. It has set a bad precedent and now several tests do it.
This is another EFI thing that I would like to resolve at some point. As previously mentioned, I understand we might be one test to check that U-Boot does actually kick up a firmware update when started, but I don't think we should be restarting willy-nilly.
We should indeed not be restarting willy-nilly. And we don't. But the test for "can we restart the system" must restart the system. That's the test. It's not changing. Testing if we can restart the system by faking the restart is not testing restarting the system.
If it were just one test I would be OK with it...but it has spread, unfortunately. When I run under gdb I constantly have to manually restart U-Boot.
Then I'm sorry to say you picked a bad example to bring this up on.
Hi Tom,
On Sun, 13 Oct 2024 at 20:25, Tom Rini trini@konsulko.com wrote:
On Sun, Oct 13, 2024 at 01:33:32PM -0600, Simon Glass wrote:
Hi Tom,
On Fri, 11 Oct 2024 at 16:28, Tom Rini trini@konsulko.com wrote:
On Fri, Oct 11, 2024 at 04:16:28PM -0600, Simon Glass wrote:
Hi Heinrich, Tom,
On Tue, 1 Oct 2024 at 16:18, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
[snip]
The relevant error message is:
E Exception: Bad pattern found on console: spl_signonThe error occurs because the substring
U-Boot SPL 2024.10occurs multiple times during the reboot process. This is not expected in wait_for_boot_prompt().
The EFI test is resetting the board and then calling restart_uboot() which by itself does another reset.
We had this discussion a while back and I was very-much opposed to restarting U-Boot within a test. It has set a bad precedent and now several tests do it.
This is another EFI thing that I would like to resolve at some point. As previously mentioned, I understand we might be one test to check that U-Boot does actually kick up a firmware update when started, but I don't think we should be restarting willy-nilly.
We should indeed not be restarting willy-nilly. And we don't. But the test for "can we restart the system" must restart the system. That's the test. It's not changing. Testing if we can restart the system by faking the restart is not testing restarting the system.
If it were just one test I would be OK with it...but it has spread, unfortunately. When I run under gdb I constantly have to manually restart U-Boot.
Then I'm sorry to say you picked a bad example to bring this up on.
Ha ha, yes! I will resend the series without the skipping.
- Simon
On Tue, 1 Oct 2024 at 01:42, Tom Rini trini@konsulko.com wrote:
On Tue, Oct 01, 2024 at 01:38:56AM +0200, Heinrich Schuchardt wrote:
On 26.09.24 23:59, 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
Please, consider prior review before resubmitting patches.
As responded to all prior submissions:
We want to test the code running on actual machines. We don't want to have sandbox code everywhere.
I cannot see any test that is not passing due to the current behavior.
The pytests for the EFI selftests are unreliable for me, on Raspberry Pi 3, more often in 32bit mode than 64bit mode, but I feel like I see it there too. And when they fail, the console log is full of ANSI escape sequences. Is this specific test a test you run regularly on real hardware?
Is it possible there's a bug in the RPi support somewhere that's causing these issues that is triggered more easily with the ASCI codes?
On Tue, Oct 01, 2024 at 08:49:26AM +0100, Peter Robinson wrote:
On Tue, 1 Oct 2024 at 01:42, Tom Rini trini@konsulko.com wrote:
On Tue, Oct 01, 2024 at 01:38:56AM +0200, Heinrich Schuchardt wrote:
On 26.09.24 23:59, 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
Please, consider prior review before resubmitting patches.
As responded to all prior submissions:
We want to test the code running on actual machines. We don't want to have sandbox code everywhere.
I cannot see any test that is not passing due to the current behavior.
The pytests for the EFI selftests are unreliable for me, on Raspberry Pi 3, more often in 32bit mode than 64bit mode, but I feel like I see it there too. And when they fail, the console log is full of ANSI escape sequences. Is this specific test a test you run regularly on real hardware?
Is it possible there's a bug in the RPi support somewhere that's causing these issues that is triggered more easily with the ASCI codes?
I don't think so, in that I thought I saw it on other platforms as well, but I don't have the logs anymore. I'll see if I can trigger a failure again.
Hi Heinrich,
On Mon, 30 Sept 2024 at 17:44, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 26.09.24 23:59, 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
Please, consider prior review before resubmitting patches.
I'm just coming back to this and trying one more time to resolve this problem.
As responded to all prior submissions:
We want to test the code running on actual machines. We don't want to have sandbox code everywhere.
Who is 'we'? U-Boot features have to be designed for testing. It is not a black box. We can and do have special code for sandbox in U-Boot. We also have the ability to adjust how real boards run in tests, e.g. collecting console output. The whole sandbox construct is designed for testing and development.
I cannot see any test that is not passing due to the current behavior.
This series includes a test which fails due to the unwanted ANSI output. It is the last patch in this series.
The size serial console is just requested once in the live-time of the U-Boot session.
When I use test.py with EFI it seems to happen quite a lot.
You have bunches of options in your upcoming tests. Neither of these requires the suggested patch:
- You can set stdout=vidconsole,serial and let the vidconsole provide a
screen size.
Do you mean manually change the default console and rebuild U-Boot? That is not really relevant to the problem here, since I often generally run tests without a video console.
- You can add the ANSI sequence to your ut_assert_nextline() statement.
Yes, that is feasible, but very strange and brittle. I'll give this a try for the next version.
- You can filter out ANSI codes in the test framework.
Do you mean in the assert_nextline() functions, or in Python?
- You can run an EFI command like 'printenv -e' before the command that
you actually want to test.
How do I inject that command when running test.py?
I appreciate you taking the time to list some options. But the approach here is just not right, unfortunately.
Here is another example. When I run EFI tests (e.g. test.py with '-k efi') I get something like this after the tests finish:
^[[36;90R^[[36;90R^[[36;90R^[[36;90R^[[36;90R^[[36;90R^[[36;90R^[[36;90R^[[36;90R^[[36;90R^[[36;90R^[[36;90R✘
I have to clear it from my console, before I can do anything else. If I press 'enter' I get:
bash: syntax error near unexpected token `;'
I really don't want this output. The problem is not about filtering it, it is about not generating it in the first place. You can see this appearing in CI also.
No other subsystem generates ANSI characters when U-Boot starts. Why are we creating this problem in the EFI implementation?
So please, I very-much want to have a way to stop this output from being generated, not to filter it afterwards. As above, we should design U-Boot for easy testing, not treat it as a black box. Please do seriously consider this as I believe this is an important testing principle being violated here.
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;elseret = query_console_serial(&rows, &cols);} if (ret) return;
On Fri, Oct 11, 2024 at 04:18:07PM -0600, Simon Glass wrote:
[snip]
No other subsystem generates ANSI characters when U-Boot starts. Why are we creating this problem in the EFI implementation?
So please, I very-much want to have a way to stop this output from being generated, not to filter it afterwards. As above, we should design U-Boot for easy testing, not treat it as a black box. Please do seriously consider this as I believe this is an important testing principle being violated here.
This is, I think, a good point. Outside of user-facing interactive code, we don't use ANSI sequences for printing information in U-Boot anywhere else. Why are we doing it in the EFI_LOADER code outside of personal preference? And, can we just not instead? It does confuse matters (as my example showed) when problems do arise.
Am 12. Oktober 2024 00:54:38 MESZ schrieb Tom Rini trini@konsulko.com:
On Fri, Oct 11, 2024 at 04:18:07PM -0600, Simon Glass wrote:
[snip]
No other subsystem generates ANSI characters when U-Boot starts. Why are we creating this problem in the EFI implementation?
So please, I very-much want to have a way to stop this output from being generated, not to filter it afterwards. As above, we should design U-Boot for easy testing, not treat it as a black box. Please do seriously consider this as I believe this is an important testing principle being violated here.
This is, I think, a good point. Outside of user-facing interactive code, we don't use ANSI sequences for printing information in U-Boot anywhere else. Why are we doing it in the EFI_LOADER code outside of personal preference? And, can we just not instead? It does confuse matters (as my example showed) when problems do arise.
Tests should be executable on any U-Boot instance. I see no virtue in a sandbox only hack.
Best regards
Heinrich
On Sat, Oct 12, 2024 at 01:45:10AM +0200, Heinrich Schuchardt wrote:
Am 12. Oktober 2024 00:54:38 MESZ schrieb Tom Rini trini@konsulko.com:
On Fri, Oct 11, 2024 at 04:18:07PM -0600, Simon Glass wrote:
[snip]
No other subsystem generates ANSI characters when U-Boot starts. Why are we creating this problem in the EFI implementation?
So please, I very-much want to have a way to stop this output from being generated, not to filter it afterwards. As above, we should design U-Boot for easy testing, not treat it as a black box. Please do seriously consider this as I believe this is an important testing principle being violated here.
This is, I think, a good point. Outside of user-facing interactive code, we don't use ANSI sequences for printing information in U-Boot anywhere else. Why are we doing it in the EFI_LOADER code outside of personal preference? And, can we just not instead? It does confuse matters (as my example showed) when problems do arise.
Tests should be executable on any U-Boot instance. I see no virtue in a sandbox only hack.
Right, and I want to know what the particular problematic escape sequences are because right now it does, on hardware, when things fail, put a bunch of un-rendered escape sequences on the console. That's why I was "What's going on?" and not seeing that it was the banner count issue.
participants (5)
-
Heinrich Schuchardt -
Ilias Apalodimas -
Peter Robinson -
Simon Glass -
Tom Rini