[PATCH v6 00/12] efi: Add a test for EFI bootmeth

[PATCH v6 00/12] efi: Add a test for EFI bootmeth

[Simon Glass]

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

-- 
2.43.0

[PATCH v6 01/12] efi_loader: Rename and move CMD_BOOTEFI_HELLO_COMPILE

[Simon Glass]

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

[PATCH v6 02/12] efi: arm: x86: riscv: Drop crt0/relocal extra- rules

[Simon Glass]

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

[PATCH v6 03/12] efi_loader: Shorten the app rules

[Simon Glass]

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

[PATCH v6 04/12] efi_loader: Shorten the app rules further

[Simon Glass]

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

[PATCH v6 05/12] efi_loader: Show the vendor in helloworld

[Simon Glass]

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

[PATCH v6 06/12] efi: Use the same filename for all sandbox builds

[Simon Glass]

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

[PATCH v6 07/12] bootstd: Add debugging for efi bootmeth

[Simon Glass]

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

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

[Simon Glass]

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

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

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

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

(no changes since v1)

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

diff --git a/include/efi_loader.h b/include/efi_loader.h
index f84852e384f..82b90ee0f1d 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -531,8 +531,27 @@ efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index);
 efi_status_t efi_bootmgr_run(void *fdt);
 /* search the boot option index in BootOrder */
 bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *index);
-/* Set up console modes */
+
+/**
+ * efi_setup_console_size() - update the mode table.
+ *
+ * By default the only mode available is 80x25. If the console has at least 50
+ * lines, enable mode 80x50. If we can query the console size and it is neither
+ * 80x25 nor 80x50, set it as an additional mode.
+ */
 void efi_setup_console_size(void);
+
+/**
+ * efi_console_set_ansi() - Set whether ANSI characters should be emitted
+ *
+ * These characters mess up tests which use ut_assert_nextline(). Call this
+ * function to tell efi_loader not to emit these characters when starting up the
+ * terminal
+ *
+ * @allow_ansi: Allow emitting ANSI characters
+ */
+void efi_console_set_ansi(bool allow_ansi);
+
 /* Set up load options from environment variable */
 efi_status_t efi_env_set_load_options(efi_handle_t handle, const char *env_var,
 				      u16 **load_options);
diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
index cea50c748aa..569fc9199bc 100644
--- a/lib/efi_loader/efi_console.c
+++ b/lib/efi_loader/efi_console.c
@@ -30,6 +30,17 @@ struct cout_mode {
 
 __maybe_unused static struct efi_object uart_obj;
 
+/*
+ * suppress emission of ANSI codes for use by unit tests. Leave it as 0 for the
+ * default behaviour
+ */
+static bool no_ansi;
+
+void efi_console_set_ansi(bool allow_ansi)
+{
+	no_ansi = !allow_ansi;
+}
+
 static struct cout_mode efi_cout_modes[] = {
 	/* EFI Mode 0 is 80x25 and always present */
 	{
@@ -348,13 +359,6 @@ static int __maybe_unused query_vidconsole(int *rows, int *cols)
 	return 0;
 }
 
-/**
- * efi_setup_console_size() - update the mode table.
- *
- * By default the only mode available is 80x25. If the console has at least 50
- * lines, enable mode 80x50. If we can query the console size and it is neither
- * 80x25 nor 80x50, set it as an additional mode.
- */
 void efi_setup_console_size(void)
 {
 	int rows = 25, cols = 80;
@@ -362,8 +366,12 @@ void efi_setup_console_size(void)
 
 	if (IS_ENABLED(CONFIG_VIDEO))
 		ret = query_vidconsole(&rows, &cols);
-	if (ret)
-		ret = query_console_serial(&rows, &cols);
+	if (ret) {
+		if (no_ansi)
+			ret = 0;
+		else
+			ret = query_console_serial(&rows, &cols);
+	}
 	if (ret)
 		return;
 
-- 
2.43.0

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

[Simon Glass]

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

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

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

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

(no changes since v1)

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

diff --git a/include/efi_loader.h b/include/efi_loader.h
index f84852e384f..82b90ee0f1d 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -531,8 +531,27 @@ efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index);
 efi_status_t efi_bootmgr_run(void *fdt);
 /* search the boot option index in BootOrder */
 bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *index);
-/* Set up console modes */
+
+/**
+ * efi_setup_console_size() - update the mode table.
+ *
+ * By default the only mode available is 80x25. If the console has at least 50
+ * lines, enable mode 80x50. If we can query the console size and it is neither
+ * 80x25 nor 80x50, set it as an additional mode.
+ */
 void efi_setup_console_size(void);
+
+/**
+ * efi_console_set_ansi() - Set whether ANSI characters should be emitted
+ *
+ * These characters mess up tests which use ut_assert_nextline(). Call this
+ * function to tell efi_loader not to emit these characters when starting up the
+ * terminal
+ *
+ * @allow_ansi: Allow emitting ANSI characters
+ */
+void efi_console_set_ansi(bool allow_ansi);
+
 /* Set up load options from environment variable */
 efi_status_t efi_env_set_load_options(efi_handle_t handle, const char *env_var,
 				      u16 **load_options);
diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
index cea50c748aa..569fc9199bc 100644
--- a/lib/efi_loader/efi_console.c
+++ b/lib/efi_loader/efi_console.c
@@ -30,6 +30,17 @@ struct cout_mode {
 
 __maybe_unused static struct efi_object uart_obj;
 
+/*
+ * suppress emission of ANSI codes for use by unit tests. Leave it as 0 for the
+ * default behaviour
+ */
+static bool no_ansi;
+
+void efi_console_set_ansi(bool allow_ansi)
+{
+	no_ansi = !allow_ansi;
+}
+
 static struct cout_mode efi_cout_modes[] = {
 	/* EFI Mode 0 is 80x25 and always present */
 	{
@@ -348,13 +359,6 @@ static int __maybe_unused query_vidconsole(int *rows, int *cols)
 	return 0;
 }
 
-/**
- * efi_setup_console_size() - update the mode table.
- *
- * By default the only mode available is 80x25. If the console has at least 50
- * lines, enable mode 80x50. If we can query the console size and it is neither
- * 80x25 nor 80x50, set it as an additional mode.
- */
 void efi_setup_console_size(void)
 {
 	int rows = 25, cols = 80;
@@ -362,8 +366,12 @@ void efi_setup_console_size(void)
 
 	if (IS_ENABLED(CONFIG_VIDEO))
 		ret = query_vidconsole(&rows, &cols);
-	if (ret)
-		ret = query_console_serial(&rows, &cols);
+	if (ret) {
+		if (no_ansi)
+			ret = 0;
+		else
+			ret = query_console_serial(&rows, &cols);
+	}
 	if (ret)
 		return;
 
-- 
2.43.0

Re: [PATCH v6 05/12] efi_loader: Show the vendor in helloworld

[Ilias Apalodimas]

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+DtHbghAqJWiwm8ZVCXf3mCOOm13YpLSQGGBPcpgnWiQ@mail.gmail.com/

Re: [PATCH v6 05/12] efi_loader: Show the vendor in helloworld

[Simon Glass]

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+DtHbghAqJWiwm8ZVCXf3mCOOm13YpLSQGGBPcpgnWiQ@mail.gmail.com/

Regards,
Simon

Re: [PATCH v6 06/12] efi: Use the same filename for all sandbox builds

[Heinrich Schuchardt]

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

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

[Heinrich Schuchardt]

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;
> +		else
> +			ret = query_console_serial(&rows, &cols);
> +	}
>   	if (ret)
>   		return;
>

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

[Tom Rini]

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

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?

-- 
Tom

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

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

[Heinrich Schuchardt]

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

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

[Peter Robinson]

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?

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

[Tom Rini]

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

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.

-- 
Tom

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

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

[Tom Rini]

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

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^[[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
\0?
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.

-- 
Tom

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

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

[Heinrich Schuchardt]

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

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

[Tom Rini]

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

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^[[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_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.

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
> 

-- 
Tom

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

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

[Simon Glass]

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

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

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

[Simon Glass]

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;
> > +             else
> > +                     ret = query_console_serial(&rows, &cols);
> > +     }
> >       if (ret)
> >               return;
> >
>

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

[Tom Rini]

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

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

-- 
Tom

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

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

[Tom Rini]

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

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.

-- 
Tom

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

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

[Heinrich Schuchardt]

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

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

[Tom Rini]

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

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.

-- 
Tom

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

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

[Simon Glass]

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

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

[Tom Rini]

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

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

-- 
Tom

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

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

[Simon Glass]

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

Re: [PATCH v6 06/12] efi: Use the same filename for all sandbox builds

[Simon Glass]

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

Re: [PATCH v6 06/12] efi: Use the same filename for all sandbox builds

[Tom Rini]

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

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

-- 
Tom

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

Re: [PATCH v6 06/12] efi: Use the same filename for all sandbox builds

[Simon Glass]

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

Re: [PATCH v6 06/12] efi: Use the same filename for all sandbox builds

[Heinrich Schuchardt]

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

Re: [PATCH v6 06/12] efi: Use the same filename for all sandbox builds

[Simon Glass]

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

Re: [PATCH v6 06/12] efi: Use the same filename for all sandbox builds

[Heinrich Schuchardt]

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