[U-Boot] [PATCH v3 1/8] x86: Add implementations of setjmp() and longjmp()

[U-Boot] [PATCH v3 1/8] x86: Add implementations of setjmp() and longjmp()

[Simon Glass]

Bring in these functions from Linux v4.4. They will be needed for EFI loader
support.

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

Changes in v3:
- Add a parameter to longjmp()
- Drop unused header files

Changes in v2:
- Drop irrelevant comment
- Add a comment about .size
- Drop unnecessary .text directive
- Make longjmp() always cause setjmp() to return 1

 arch/x86/cpu/Makefile         |  2 +-
 arch/x86/cpu/setjmp.S         | 61 +++++++++++++++++++++++++++++++++++++++++++
 arch/x86/include/asm/setjmp.h | 24 +++++++++++++++++
 3 files changed, 86 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/cpu/setjmp.S
 create mode 100644 arch/x86/include/asm/setjmp.h

diff --git a/arch/x86/cpu/Makefile b/arch/x86/cpu/Makefile
index 2667e0b..f5b8c9e 100644
--- a/arch/x86/cpu/Makefile
+++ b/arch/x86/cpu/Makefile
@@ -10,7 +10,7 @@
 
 extra-y	= start.o
 obj-$(CONFIG_X86_RESET_VECTOR) += resetvec.o start16.o
-obj-y	+= interrupts.o cpu.o cpu_x86.o call64.o
+obj-y	+= interrupts.o cpu.o cpu_x86.o call64.o setjmp.o
 
 AFLAGS_REMOVE_call32.o := -mregparm=3 \
 	$(if $(CONFIG_EFI_STUB_64BIT),-march=i386 -m32)
diff --git a/arch/x86/cpu/setjmp.S b/arch/x86/cpu/setjmp.S
new file mode 100644
index 0000000..2ea1c6c
--- /dev/null
+++ b/arch/x86/cpu/setjmp.S
@@ -0,0 +1,61 @@
+/*
+ * Written by H. Peter Anvin <hpa@zytor.com>
+ * Brought in from Linux v4.4 and modified for U-Boot
+ * From Linux arch/um/sys-i386/setjmp.S
+ *
+ * SPDX-License-Identifier:	GPL-2.0
+ */
+
+#define _REGPARM
+
+/*
+ * The jmp_buf is assumed to contain the following, in order:
+ *	%ebx
+ *	%esp
+ *	%ebp
+ *	%esi
+ *	%edi
+ *	<return address>
+ */
+
+	.text
+	.align 4
+	.globl setjmp
+	.type setjmp, @function
+setjmp:
+#ifdef _REGPARM
+	movl %eax, %edx
+#else
+	movl 4(%esp), %edx
+#endif
+	popl %ecx		/* Return address, and adjust the stack */
+	xorl %eax, %eax		/* Return value */
+	movl %ebx, (%edx)
+	movl %esp, 4(%edx)	/* Post-return %esp! */
+	pushl %ecx		/* Make the call/return stack happy */
+	movl %ebp, 8(%edx)
+	movl %esi, 12(%edx)
+	movl %edi, 16(%edx)
+	movl %ecx, 20(%edx)	/* Return address */
+	ret
+
+	/* Provide function size if needed */
+	.size setjmp, .-setjmp
+
+	.align 4
+	.globl longjmp
+	.type longjmp, @function
+longjmp:
+#ifdef _REGPARM
+	xchgl %eax, %edx
+#else
+	movl 4(%esp), %edx	/* jmp_ptr address */
+#endif
+	movl (%edx), %ebx
+	movl 4(%edx), %esp
+	movl 8(%edx), %ebp
+	movl 12(%edx), %esi
+	movl 16(%edx), %edi
+	jmp *20(%edx)
+
+	.size longjmp, .-longjmp
diff --git a/arch/x86/include/asm/setjmp.h b/arch/x86/include/asm/setjmp.h
new file mode 100644
index 0000000..1feaa89
--- /dev/null
+++ b/arch/x86/include/asm/setjmp.h
@@ -0,0 +1,24 @@
+/*
+ * Written by H. Peter Anvin <hpa@zytor.com>
+ * Brought in from Linux v4.4 and modified for U-Boot
+ * From Linux arch/um/sys-i386/setjmp.S
+ *
+ * SPDX-License-Identifier:	GPL-2.0
+ */
+
+#ifndef __setjmp_h
+#define __setjmp_h
+
+struct jmp_buf_data {
+	unsigned int __ebx;
+	unsigned int __esp;
+	unsigned int __ebp;
+	unsigned int __esi;
+	unsigned int __edi;
+	unsigned int __eip;
+};
+
+int setjmp(struct jmp_buf_data *jmp_buf);
+void longjmp(struct jmp_buf_data *jmp_buf, int val);
+
+#endif
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH v3 2/8] efi: Use asmlinkage for EFIAPI

[Simon Glass]

This is required for x86 and is also correct for ARM (since it is empty).

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---

Changes in v3: None
Changes in v2:
- Move efi.h changes to a new patch

 arch/avr32/include/asm/linkage.h      | 0
 arch/m68k/include/asm/linkage.h       | 0
 arch/microblaze/include/asm/linkage.h | 0
 arch/mips/include/asm/linkage.h       | 0
 arch/nios2/include/asm/linkage.h      | 0
 arch/openrisc/include/asm/linkage.h   | 0
 arch/sandbox/include/asm/linkage.h    | 0
 arch/sh/include/asm/linkage.h         | 0
 arch/sparc/include/asm/linkage.h      | 0
 include/efi.h                         | 3 ++-
 10 files changed, 2 insertions(+), 1 deletion(-)
 create mode 100644 arch/avr32/include/asm/linkage.h
 create mode 100644 arch/m68k/include/asm/linkage.h
 create mode 100644 arch/microblaze/include/asm/linkage.h
 create mode 100644 arch/mips/include/asm/linkage.h
 create mode 100644 arch/nios2/include/asm/linkage.h
 create mode 100644 arch/openrisc/include/asm/linkage.h
 create mode 100644 arch/sandbox/include/asm/linkage.h
 create mode 100644 arch/sh/include/asm/linkage.h
 create mode 100644 arch/sparc/include/asm/linkage.h

diff --git a/arch/avr32/include/asm/linkage.h b/arch/avr32/include/asm/linkage.h
new file mode 100644
index 0000000..e69de29
diff --git a/arch/m68k/include/asm/linkage.h b/arch/m68k/include/asm/linkage.h
new file mode 100644
index 0000000..e69de29
diff --git a/arch/microblaze/include/asm/linkage.h b/arch/microblaze/include/asm/linkage.h
new file mode 100644
index 0000000..e69de29
diff --git a/arch/mips/include/asm/linkage.h b/arch/mips/include/asm/linkage.h
new file mode 100644
index 0000000..e69de29
diff --git a/arch/nios2/include/asm/linkage.h b/arch/nios2/include/asm/linkage.h
new file mode 100644
index 0000000..e69de29
diff --git a/arch/openrisc/include/asm/linkage.h b/arch/openrisc/include/asm/linkage.h
new file mode 100644
index 0000000..e69de29
diff --git a/arch/sandbox/include/asm/linkage.h b/arch/sandbox/include/asm/linkage.h
new file mode 100644
index 0000000..e69de29
diff --git a/arch/sh/include/asm/linkage.h b/arch/sh/include/asm/linkage.h
new file mode 100644
index 0000000..e69de29
diff --git a/arch/sparc/include/asm/linkage.h b/arch/sparc/include/asm/linkage.h
new file mode 100644
index 0000000..e69de29
diff --git a/include/efi.h b/include/efi.h
index 5a3b8cf..d07187c 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -15,6 +15,7 @@
 #ifndef _EFI_H
 #define _EFI_H
 
+#include <linux/linkage.h>
 #include <linux/string.h>
 #include <linux/types.h>
 
@@ -22,7 +23,7 @@
 /* EFI uses the Microsoft ABI which is not the default for GCC */
 #define EFIAPI __attribute__((ms_abi))
 #else
-#define EFIAPI
+#define EFIAPI asmlinkage
 #endif
 
 struct efi_device_path;
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH v3 3/8] efi: Fix missing EFIAPI specifiers

[Simon Glass]

These are missing in some functions. Add them to keep things consistent.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Reviewed-by: Alexander Graf <agraf@suse.de>
---

Changes in v3: None
Changes in v2: None

 cmd/bootefi.c                 |  7 +++++--
 include/efi_loader.h          |  2 +-
 lib/efi_loader/efi_boottime.c |  5 +++--
 lib/efi_loader/efi_disk.c     | 13 +++++++------
 lib/efi_loader/efi_net.c      |  4 ++--
 5 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 21fe42c..7b2e0b5 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -15,6 +15,8 @@
 #include <libfdt_env.h>
 #include <memalign.h>
 #include <asm/global_data.h>
+#include <asm-generic/sections.h>
+#include <linux/linkage.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -52,7 +54,7 @@ static struct efi_device_path_file_path bootefi_device_path[] = {
 	}
 };
 
-static efi_status_t bootefi_open_dp(void *handle, efi_guid_t *protocol,
+static efi_status_t EFIAPI bootefi_open_dp(void *handle, efi_guid_t *protocol,
 			void **protocol_interface, void *agent_handle,
 			void *controller_handle, uint32_t attributes)
 {
@@ -145,7 +147,8 @@ static void *copy_fdt(void *fdt)
  */
 static unsigned long do_bootefi_exec(void *efi, void *fdt)
 {
-	ulong (*entry)(void *image_handle, struct efi_system_table *st);
+	ulong (*entry)(void *image_handle, struct efi_system_table *st)
+		asmlinkage;
 	ulong fdt_pages, fdt_size, fdt_start, fdt_end;
 	bootm_headers_t img = { 0 };
 
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 9738835..aa4ae0e 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -93,7 +93,7 @@ void efi_net_set_dhcp_ack(void *pkt, int len);
  * Stub implementation for a protocol opener that just returns the handle as
  * interface
  */
-efi_status_t efi_return_handle(void *handle,
+efi_status_t EFIAPI efi_return_handle(void *handle,
 		efi_guid_t *protocol, void **protocol_interface,
 		void *agent_handle, void *controller_handle,
 		uint32_t attributes);
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 792db39..01a9b8f 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -159,7 +159,7 @@ static struct {
 	u32 trigger_time;
 	u64 trigger_next;
 	unsigned long notify_tpl;
-	void (*notify_function) (void *event, void *context);
+	void (EFIAPI *notify_function) (void *event, void *context);
 	void *notify_context;
 } efi_event = {
 	/* Disable timers on bootup */
@@ -168,7 +168,8 @@ static struct {
 
 static efi_status_t EFIAPI efi_create_event(
 			enum efi_event_type type, ulong notify_tpl,
-			void (*notify_function) (void *event, void *context),
+			void (EFIAPI *notify_function) (void *event,
+							void *context),
 			void *notify_context, void **event)
 {
 	EFI_ENTRY("%d, 0x%lx, %p, %p", type, notify_tpl, notify_function,
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index d8ddcc9..1e3dca4 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -35,9 +35,10 @@ struct efi_disk_obj {
 	const struct blk_desc *desc;
 };
 
-static efi_status_t efi_disk_open_block(void *handle, efi_guid_t *protocol,
-			void **protocol_interface, void *agent_handle,
-			void *controller_handle, uint32_t attributes)
+static efi_status_t EFIAPI efi_disk_open_block(void *handle,
+			efi_guid_t *protocol, void **protocol_interface,
+			void *agent_handle, void *controller_handle,
+			uint32_t attributes)
 {
 	struct efi_disk_obj *diskobj = handle;
 
@@ -46,7 +47,7 @@ static efi_status_t efi_disk_open_block(void *handle, efi_guid_t *protocol,
 	return EFI_SUCCESS;
 }
 
-static efi_status_t efi_disk_open_dp(void *handle, efi_guid_t *protocol,
+static efi_status_t EFIAPI efi_disk_open_dp(void *handle, efi_guid_t *protocol,
 			void **protocol_interface, void *agent_handle,
 			void *controller_handle, uint32_t attributes)
 {
@@ -108,7 +109,7 @@ static efi_status_t EFIAPI efi_disk_rw_blocks(struct efi_block_io *this,
 	return EFI_EXIT(EFI_SUCCESS);
 }
 
-static efi_status_t efi_disk_read_blocks(struct efi_block_io *this,
+static efi_status_t EFIAPI efi_disk_read_blocks(struct efi_block_io *this,
 			u32 media_id, u64 lba, unsigned long buffer_size,
 			void *buffer)
 {
@@ -143,7 +144,7 @@ static efi_status_t efi_disk_read_blocks(struct efi_block_io *this,
 	return EFI_EXIT(r);
 }
 
-static efi_status_t efi_disk_write_blocks(struct efi_block_io *this,
+static efi_status_t EFIAPI efi_disk_write_blocks(struct efi_block_io *this,
 			u32 media_id, u64 lba, unsigned long buffer_size,
 			void *buffer)
 {
diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
index dd3b485..9199518 100644
--- a/lib/efi_loader/efi_net.c
+++ b/lib/efi_loader/efi_net.c
@@ -191,7 +191,7 @@ static efi_status_t EFIAPI efi_net_receive(struct efi_simple_network *this,
 	return EFI_EXIT(EFI_SUCCESS);
 }
 
-static efi_status_t efi_net_open_dp(void *handle, efi_guid_t *protocol,
+static efi_status_t EFIAPI efi_net_open_dp(void *handle, efi_guid_t *protocol,
 			void **protocol_interface, void *agent_handle,
 			void *controller_handle, uint32_t attributes)
 {
@@ -203,7 +203,7 @@ static efi_status_t efi_net_open_dp(void *handle, efi_guid_t *protocol,
 	return EFI_SUCCESS;
 }
 
-static efi_status_t efi_net_open_pxe(void *handle, efi_guid_t *protocol,
+static efi_status_t EFIAPI efi_net_open_pxe(void *handle, efi_guid_t *protocol,
 			void **protocol_interface, void *agent_handle,
 			void *controller_handle, uint32_t attributes)
 {
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH v3 4/8] x86: Tidy up selection of building the EFI stub

[Simon Glass]

At present we use a CONFIG option in efi.h to determine whether we are
building the EFI stub or not. This means that the same header cannot be
used for EFI_LOADER support. The CONFIG option will be enabled for the
whole build, even when not building the stub.

Use a different define instead, set up just for the files that make up the
stub.

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

Changes in v3: None
Changes in v2:
- Add new patch to tidy up selection of building the EFI stub

 include/efi.h    | 7 +++++--
 lib/efi/Makefile | 4 ++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/include/efi.h b/include/efi.h
index d07187c..3d58780 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -30,8 +30,11 @@ struct efi_device_path;
 
 #define EFI_BITS_PER_LONG	BITS_PER_LONG
 
-/* With 64-bit EFI stub, EFI_BITS_PER_LONG has to be 64 */
-#ifdef CONFIG_EFI_STUB_64BIT
+/*
+ * With 64-bit EFI stub, EFI_BITS_PER_LONG has to be 64. EFI_STUB is set
+ * in lib/efi/Makefile, when building the stub.
+ */
+#if defined(CONFIG_EFI_STUB_64BIT) && defined(EFI_STUB)
 #undef EFI_BITS_PER_LONG
 #define EFI_BITS_PER_LONG	64
 #endif
diff --git a/lib/efi/Makefile b/lib/efi/Makefile
index e32dc14..9449600 100644
--- a/lib/efi/Makefile
+++ b/lib/efi/Makefile
@@ -9,9 +9,9 @@ obj-$(CONFIG_EFI_STUB) += efi_info.o
 
 CFLAGS_REMOVE_efi_stub.o := -mregparm=3 \
 	$(if $(CONFIG_EFI_STUB_64BIT),-march=i386 -m32)
-CFLAGS_efi_stub.o := -fpic -fshort-wchar
+CFLAGS_efi_stub.o := -fpic -fshort-wchar -DEFI_STUB
 CFLAGS_REMOVE_efi.o := -mregparm=3 \
 	$(if $(CONFIG_EFI_STUB_64BIT),-march=i386 -m32)
-CFLAGS_efi.o := -fpic -fshort-wchar
+CFLAGS_efi.o := -fpic -fshort-wchar -DEFI_STUB
 
 extra-$(CONFIG_EFI_STUB) += efi_stub.o efi.o
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH v3 5/8] arm: efi: Add a hello world test program

[Simon Glass]

It is useful to have a basic sanity check for EFI loader support. Add a
'bootefi hello' command which loads HelloWord.efi and runs it under U-Boot.

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

Changes in v3:
- Include a link to the program instead of adding it to the tree
- Fix several typos
- Align backslashes to the same column

Changes in v2: None

 arch/arm/lib/Makefile          |  7 +++++++
 cmd/Kconfig                    |  9 +++++++++
 cmd/bootefi.c                  | 26 ++++++++++++++++++++------
 doc/README.efi                 | 22 ++++++++++++++++++++++
 include/asm-generic/sections.h |  2 ++
 scripts/Makefile.lib           | 19 +++++++++++++++++++
 6 files changed, 79 insertions(+), 6 deletions(-)

diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
index caa62c6..64378e1 100644
--- a/arch/arm/lib/Makefile
+++ b/arch/arm/lib/Makefile
@@ -30,6 +30,13 @@ obj-$(CONFIG_CMD_BOOTI) += bootm.o
 obj-$(CONFIG_CMD_BOOTM) += bootm.o
 obj-$(CONFIG_CMD_BOOTZ) += bootm.o zimage.o
 obj-$(CONFIG_SYS_L2_PL310) += cache-pl310.o
+ifdef CONFIG_ARM64
+# This option does not work for arm64, as there is no binary.
+# TODO(sjg at chromium.org): Add this once it is possible to build one
+obj-$(CONFIG_CMD_BOOTEFI_HELLO) += HelloWorld64.o
+else
+obj-$(CONFIG_CMD_BOOTEFI_HELLO) += HelloWorld32.o
+endif
 obj-$(CONFIG_USE_ARCH_MEMSET) += memset.o
 obj-$(CONFIG_USE_ARCH_MEMCPY) += memcpy.o
 else
diff --git a/cmd/Kconfig b/cmd/Kconfig
index e339d86..a5d030b 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -181,6 +181,15 @@ config CMD_BOOTEFI
 	help
 	  Boot an EFI image from memory.
 
+config CMD_BOOTEFI_HELLO
+	bool "Allow booting a standard EFI hello world for testing"
+	depends on CMD_BOOTEFI
+	help
+	  This adds a standard EFI hello world application to U-Boot so that
+	  it can be used with the 'bootefi hello' command. This is useful
+	  for testing that EFI is working at a basic level, and for bringing
+	  up EFI support on a new architecture.
+
 config CMD_ELF
 	bool "bootelf, bootvx"
 	default y
diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 7b2e0b5..7c2d9fc 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -230,13 +230,22 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 	if (argc < 2)
 		return CMD_RET_USAGE;
-	saddr = argv[1];
+#ifdef CONFIG_CMD_BOOTEFI_HELLO
+	if (!strcmp(argv[1], "hello")) {
+		addr = CONFIG_SYS_LOAD_ADDR;
+		memcpy((char *)addr, __efi_hello_world_begin,
+		       __efi_hello_world_end - __efi_hello_world_begin);
+	} else
+#endif
+	{
+		saddr = argv[1];
 
-	addr = simple_strtoul(saddr, NULL, 16);
+		addr = simple_strtoul(saddr, NULL, 16);
 
-	if (argc > 2) {
-		sfdt = argv[2];
-		fdt_addr = simple_strtoul(sfdt, NULL, 16);
+		if (argc > 2) {
+			sfdt = argv[2];
+			fdt_addr = simple_strtoul(sfdt, NULL, 16);
+		}
 	}
 
 	printf("## Starting EFI application at 0x%08lx ...\n", addr);
@@ -254,7 +263,12 @@ static char bootefi_help_text[] =
 	"<image address> [fdt address]\n"
 	"  - boot EFI payload stored at address <image address>.\n"
 	"    If specified, the device tree located at <fdt address> gets\n"
-	"    exposed as EFI configuration table.\n";
+	"    exposed as EFI configuration table.\n"
+#ifdef CONFIG_CMD_BOOTEFI_HELLO
+	"hello\n"
+	"  - boot a sample Hello World application stored within U-Boot"
+#endif
+	;
 #endif
 
 U_BOOT_CMD(
diff --git a/doc/README.efi b/doc/README.efi
index 1fd3f00..6e76310 100644
--- a/doc/README.efi
+++ b/doc/README.efi
@@ -310,6 +310,28 @@ Removable media booting (search for /efi/boot/boota{a64,arm}.efi) is supported.
 Simple use cases like "Plug this SD card into my ARM device and it just
 boots into grub which boots into Linux", work very well.
 
+
+Running HelloWord.efi
+---------------------
+
+You can run a simple 'hello world' EFI program in U-Boot. This is not
+distributed in the U-Boot source, but you can download this patch:
+
+   https://goo.gl/vihiZS
+
+Then apply it, e.g.:
+
+   $ git am ~/Downloads/0001-efi-Add-test-program-binaries.patch
+
+and enable the option CONFIG_CMD_BOOTEFI_HELLO
+
+Then you can boot into U-Boot and type:
+
+   > bootefi hello
+
+The 'hello world EFI' program will then run, print a message and exit.
+
+
 Future work
 -----------
 
diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index d69bc60..daf021b 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -22,6 +22,8 @@ extern char __kprobes_text_start[], __kprobes_text_end[];
 extern char __entry_text_start[], __entry_text_end[];
 extern char __initdata_begin[], __initdata_end[];
 extern char __start_rodata[], __end_rodata[];
+extern char __efi_hello_world_begin[];
+extern char __efi_hello_world_end[];
 
 /* Start and end of .ctors section - used for constructor calls. */
 extern char __ctors_start[], __ctors_end[];
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 45a0e1d..1f534b7 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -321,6 +321,25 @@ cmd_S_ttf=						\
 $(obj)/%.S: $(src)/%.ttf
 	$(call cmd,S_ttf)
 
+# EFI Hello World application
+# ---------------------------------------------------------------------------
+
+# Generate an assembly file to wrap the EFI app
+cmd_S_efi=						\
+(							\
+	echo '.section .rodata.efi.init,"a"';		\
+	echo '.balign 16';				\
+	echo '.global __efi_hello_world_begin';		\
+	echo '__efi_hello_world_begin:';		\
+	echo '.incbin "$<" ';				\
+	echo '__efi_hello_world_end:';			\
+	echo '.global __efi_hello_world_end';		\
+	echo '.balign 16';				\
+) > $@
+
+$(obj)/%.S: $(src)/%.efi
+	$(call cmd,S_efi)
+
 # ACPI
 # ---------------------------------------------------------------------------
 quiet_cmd_acpi_c_asl= ASL     $<
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH v3 6/8] x86: efi: Add EFI loader support for x86

[Simon Glass]

Add the required pieces to support the EFI loader on x86.

Since U-Boot only builds for 32-bit on x86, only a 32-bit EFI application
is supported. If a 64-bit kernel must be booted, U-Boot supports this
directly using FIT (see doc/uImage.FIT/kernel.its). U-Boot can act as a
payload for both 32-bit and 64-bit EFI.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---

Changes in v3: None
Changes in v2: None

 arch/x86/cpu/u-boot.lds       | 36 +++++++++++++++++++++++++++++++++++-
 arch/x86/lib/Makefile         |  1 +
 arch/x86/lib/sections.c       | 12 ++++++++++++
 lib/efi_loader/efi_boottime.c |  9 +++++++++
 lib/efi_loader/efi_runtime.c  |  4 ++++
 5 files changed, 61 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/lib/sections.c

diff --git a/arch/x86/cpu/u-boot.lds b/arch/x86/cpu/u-boot.lds
index 36f59ea..cca536b 100644
--- a/arch/x86/cpu/u-boot.lds
+++ b/arch/x86/cpu/u-boot.lds
@@ -28,7 +28,10 @@ SECTIONS
 	}
 
 	. = ALIGN(4);
-	.rodata : { *(SORT_BY_ALIGNMENT(SORT_BY_NAME(.rodata*))) }
+	.rodata : {
+		*(SORT_BY_ALIGNMENT(SORT_BY_NAME(.rodata*)))
+		KEEP(*(.rodata.efi.init));
+	}
 
 	. = ALIGN(4);
 	.data : { *(.data*) }
@@ -40,6 +43,37 @@ SECTIONS
 	.got : { *(.got*) }
 
 	. = ALIGN(4);
+
+	.__efi_runtime_start : {
+		*(.__efi_runtime_start)
+	}
+
+	.efi_runtime : {
+		*(efi_runtime_text)
+		*(efi_runtime_data)
+	}
+
+	.__efi_runtime_stop : {
+		*(.__efi_runtime_stop)
+	}
+
+	.efi_runtime_rel_start :
+	{
+		*(.__efi_runtime_rel_start)
+	}
+
+	.efi_runtime_rel : {
+		*(.relefi_runtime_text)
+		*(.relefi_runtime_data)
+	}
+
+	.efi_runtime_rel_stop :
+	{
+		*(.__efi_runtime_rel_stop)
+	}
+
+	. = ALIGN(4);
+
 	__data_end = .;
 	__init_end = .;
 
diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
index e17f0bb..e46e7f1 100644
--- a/arch/x86/lib/Makefile
+++ b/arch/x86/lib/Makefile
@@ -28,6 +28,7 @@ obj-y	+= pirq_routing.o
 obj-y	+= relocate.o
 obj-y += physmem.o
 obj-$(CONFIG_X86_RAMTEST) += ramtest.o
+obj-y	+= sections.o
 obj-y += sfi.o
 obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += smbios.o
 obj-y	+= string.o
diff --git a/arch/x86/lib/sections.c b/arch/x86/lib/sections.c
new file mode 100644
index 0000000..6455e0f
--- /dev/null
+++ b/arch/x86/lib/sections.c
@@ -0,0 +1,12 @@
+/*
+ * Copyright 2013 Albert ARIBAUD <albert.u.boot@aribaud.net>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+char __efi_runtime_start[0] __attribute__((section(".__efi_runtime_start")));
+char __efi_runtime_stop[0] __attribute__((section(".__efi_runtime_stop")));
+char __efi_runtime_rel_start[0]
+		__attribute__((section(".__efi_runtime_rel_start")));
+char __efi_runtime_rel_stop[0]
+		__attribute__((section(".__efi_runtime_rel_stop")));
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 01a9b8f..d16fb4e 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -39,6 +39,7 @@ static bool efi_is_direct_boot = true;
  */
 static struct efi_configuration_table EFI_RUNTIME_DATA efi_conf_table[1];
 
+#ifdef CONFIG_ARM
 /*
  * The "gd" pointer lives in a register on ARM and AArch64 that we declare
  * fixed when compiling U-Boot. However, the payload does not know about that
@@ -46,16 +47,20 @@ static struct efi_configuration_table EFI_RUNTIME_DATA efi_conf_table[1];
  * EFI callback entry/exit.
  */
 static volatile void *efi_gd, *app_gd;
+#endif
 
 /* Called from do_bootefi_exec() */
 void efi_save_gd(void)
 {
+#ifdef CONFIG_ARM
 	efi_gd = gd;
+#endif
 }
 
 /* Called on every callback entry */
 void efi_restore_gd(void)
 {
+#ifdef CONFIG_ARM
 	/* Only restore if we're already in EFI context */
 	if (!efi_gd)
 		return;
@@ -63,12 +68,16 @@ void efi_restore_gd(void)
 	if (gd != efi_gd)
 		app_gd = gd;
 	gd = efi_gd;
+#endif
 }
 
 /* Called on every callback exit */
 efi_status_t efi_exit_func(efi_status_t ret)
 {
+#ifdef CONFIG_ARM
 	gd = app_gd;
+#endif
+
 	return ret;
 }
 
diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 99b5ef1..3793471 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -34,6 +34,10 @@ static efi_status_t EFI_RUNTIME_TEXT EFIAPI efi_invalid_parameter(void);
 #elif defined(CONFIG_ARM)
 #define R_RELATIVE	23
 #define R_MASK		0xffULL
+#elif defined(CONFIG_X86)
+#include <asm/elf.h>
+#define R_RELATIVE	R_386_RELATIVE
+#define R_MASK		0xffULL
 #else
 #error Need to add relocation awareness
 #endif
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH v3 7/8] x86: efi: Add a hello world test program

[Simon Glass]

It is useful to have a basic sanity check for EFI loader support. Add a
'bootefi hello' command which loads HelloWord.efi and runs it under U-Boot.

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

Changes in v3:
- Include a link to the program instead of adding it to the tree

Changes in v2: None

 arch/x86/lib/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
index e46e7f1..1d9ebc0 100644
--- a/arch/x86/lib/Makefile
+++ b/arch/x86/lib/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_CMD_BOOTM) += bootm.o
 obj-y	+= cmd_boot.o
 obj-$(CONFIG_SEABIOS) += coreboot_table.o
 obj-$(CONFIG_EFI) += efi/
+obj-$(CONFIG_CMD_BOOTEFI_HELLO) += HelloWorld32.o
 obj-y	+= e820.o
 obj-y	+= gcc.o
 obj-y	+= init_helpers.o
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH v3 8/8] x86: Enable EFI loader support

[Simon Glass]

Enable this so that EFI applications (notably grub) can be run under U-Boot
on x86 platforms.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---

Changes in v3: None
Changes in v2:
- Remove EFI support from README.x86 TODO list

 configs/efi-x86_defconfig | 1 +
 doc/README.x86            | 1 -
 lib/efi_loader/Kconfig    | 2 +-
 3 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/configs/efi-x86_defconfig b/configs/efi-x86_defconfig
index b31c73b..1fe6142 100644
--- a/configs/efi-x86_defconfig
+++ b/configs/efi-x86_defconfig
@@ -34,3 +34,4 @@ CONFIG_USB=y
 CONFIG_USB_STORAGE=y
 CONFIG_USB_KEYBOARD=y
 CONFIG_EFI=y
+# CONFIG_EFI_LOADER is not set
diff --git a/doc/README.x86 b/doc/README.x86
index 6799559..a38cc1b 100644
--- a/doc/README.x86
+++ b/doc/README.x86
@@ -1077,7 +1077,6 @@ TODO List
 ---------
 - Audio
 - Chrome OS verified boot
-- Support for CONFIG_EFI_LOADER
 - Building U-Boot to run in 64-bit mode
 
 References
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 37a0dd6..8e7e8a5 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -1,6 +1,6 @@
 config EFI_LOADER
 	bool "Support running EFI Applications in U-Boot"
-	depends on (ARM64 ||?ARM) && OF_LIBFDT
+	depends on (ARM64 || ARM || X86) && OF_LIBFDT
 	default y
 	help
 	  Select this option if you want to run EFI applications (like grub2)
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH v3 1/8] x86: Add implementations of setjmp() and longjmp()

[Bin Meng]

On Tue, Oct 18, 2016 at 10:29 AM, Simon Glass <sjg@chromium.org> wrote:
> Bring in these functions from Linux v4.4. They will be needed for EFI loader
> support.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v3:
> - Add a parameter to longjmp()
> - Drop unused header files
>
> Changes in v2:
> - Drop irrelevant comment
> - Add a comment about .size
> - Drop unnecessary .text directive
> - Make longjmp() always cause setjmp() to return 1
>
>  arch/x86/cpu/Makefile         |  2 +-
>  arch/x86/cpu/setjmp.S         | 61 +++++++++++++++++++++++++++++++++++++++++++
>  arch/x86/include/asm/setjmp.h | 24 +++++++++++++++++
>  3 files changed, 86 insertions(+), 1 deletion(-)
>  create mode 100644 arch/x86/cpu/setjmp.S
>  create mode 100644 arch/x86/include/asm/setjmp.h
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[U-Boot] [PATCH v3 7/8] x86: efi: Add a hello world test program

[Bin Meng]

On Tue, Oct 18, 2016 at 10:29 AM, Simon Glass <sjg@chromium.org> wrote:
> It is useful to have a basic sanity check for EFI loader support. Add a
> 'bootefi hello' command which loads HelloWord.efi and runs it under U-Boot.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v3:
> - Include a link to the program instead of adding it to the tree
>
> Changes in v2: None
>
>  arch/x86/lib/Makefile | 1 +
>  1 file changed, 1 insertion(+)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[U-Boot] [PATCH v3 4/8] x86: Tidy up selection of building the EFI stub

[Bin Meng]

On Tue, Oct 18, 2016 at 10:29 AM, Simon Glass <sjg@chromium.org> wrote:
> At present we use a CONFIG option in efi.h to determine whether we are
> building the EFI stub or not. This means that the same header cannot be
> used for EFI_LOADER support. The CONFIG option will be enabled for the
> whole build, even when not building the stub.
>
> Use a different define instead, set up just for the files that make up the
> stub.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v3: None
> Changes in v2:
> - Add new patch to tidy up selection of building the EFI stub
>
>  include/efi.h    | 7 +++++--
>  lib/efi/Makefile | 4 ++--
>  2 files changed, 7 insertions(+), 4 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[U-Boot] [PATCH v3 1/8] x86: Add implementations of setjmp() and longjmp()

[Bin Meng]

On Tue, Oct 18, 2016 at 11:02 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> On Tue, Oct 18, 2016 at 10:29 AM, Simon Glass <sjg@chromium.org> wrote:
>> Bring in these functions from Linux v4.4. They will be needed for EFI loader
>> support.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v3:
>> - Add a parameter to longjmp()
>> - Drop unused header files
>>
>> Changes in v2:
>> - Drop irrelevant comment
>> - Add a comment about .size
>> - Drop unnecessary .text directive
>> - Make longjmp() always cause setjmp() to return 1
>>
>>  arch/x86/cpu/Makefile         |  2 +-
>>  arch/x86/cpu/setjmp.S         | 61 +++++++++++++++++++++++++++++++++++++++++++
>>  arch/x86/include/asm/setjmp.h | 24 +++++++++++++++++
>>  3 files changed, 86 insertions(+), 1 deletion(-)
>>  create mode 100644 arch/x86/cpu/setjmp.S
>>  create mode 100644 arch/x86/include/asm/setjmp.h
>>
>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Checked patchwork record, looks Alex has applied v2 series patch#2,
#3, #6 into his efi-next tree
(https://github.com/agraf/u-boot/tree/efi-next)

I will only pick up this v3 patch#1 via u-boot-x86 tree, while leaving
other patches in v3 to Alex to handle.

applied to u-boot-x86, thanks!

[U-Boot] [PATCH v3 2/8] efi: Use asmlinkage for EFIAPI

[Alexander Graf]

On 10/18/2016 04:29 AM, Simon Glass wrote:
> This is required for x86 and is also correct for ARM (since it is empty).
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

(Replying here in lack for a cover letter)

Could you please rebase your patches on top of

   https://github.com/agraf/u-boot.git efi-next

so that all the patches that I already queued are not repeated in the 
patch set and we don't get any conflicts?


Thanks!

Alex

[U-Boot] [PATCH v3 7/8] x86: efi: Add a hello world test program

[Alexander Graf]

On 10/18/2016 04:29 AM, Simon Glass wrote:
> It is useful to have a basic sanity check for EFI loader support. Add a
> 'bootefi hello' command which loads HelloWord.efi and runs it under U-Boot.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v3:
> - Include a link to the program instead of adding it to the tree

So, uh, where is the link?

I'm really not convinced this command buys us anything yet. I do agree 
that we want automated testing - but can't we get that using QEMU and a 
downloadable image file that we pass in as disk and have the distro boot 
do its magic?


Alex

[U-Boot] [PATCH v3 2/8] efi: Use asmlinkage for EFIAPI

[Simon Glass]

Hi Alex,

On 18 October 2016 at 01:12, Alexander Graf <agraf@suse.de> wrote:
> On 10/18/2016 04:29 AM, Simon Glass wrote:
>>
>> This is required for x86 and is also correct for ARM (since it is empty).
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>
>
> (Replying here in lack for a cover letter)
>
> Could you please rebase your patches on top of
>
>   https://github.com/agraf/u-boot.git efi-next
>
> so that all the patches that I already queued are not repeated in the patch
> set and we don't get any conflicts?

I can do that - but is this targeting -next? I was expecting these
patches to land in master.

Regards,
Simon

[U-Boot] [PATCH v3 7/8] x86: efi: Add a hello world test program

[Simon Glass]

Hi Alex,

On 18 October 2016 at 01:14, Alexander Graf <agraf@suse.de> wrote:
> On 10/18/2016 04:29 AM, Simon Glass wrote:
>>
>> It is useful to have a basic sanity check for EFI loader support. Add a
>> 'bootefi hello' command which loads HelloWord.efi and runs it under
>> U-Boot.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v3:
>> - Include a link to the program instead of adding it to the tree
>
>
> So, uh, where is the link?

I put it in the README (see the arm patch).

>
> I'm really not convinced this command buys us anything yet. I do agree that
> we want automated testing - but can't we get that using QEMU and a
> downloadable image file that we pass in as disk and have the distro boot do
> its magic?

That seems very heavyweight as a sanity check, although I agree it is useful.

Here I am just making sure that EFI programs can start, print output
and exit. It is a test that we can easily run without a lot of
overhead, much less than a full distro boot.

Regards,
Simon

[U-Boot] [PATCH v3 7/8] x86: efi: Add a hello world test program

[Alexander Graf]

On 18/10/2016 22:37, Simon Glass wrote:
> Hi Alex,
> 
> On 18 October 2016 at 01:14, Alexander Graf <agraf@suse.de> wrote:
>> On 10/18/2016 04:29 AM, Simon Glass wrote:
>>>
>>> It is useful to have a basic sanity check for EFI loader support. Add a
>>> 'bootefi hello' command which loads HelloWord.efi and runs it under
>>> U-Boot.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>
>>> Changes in v3:
>>> - Include a link to the program instead of adding it to the tree
>>
>>
>> So, uh, where is the link?
> 
> I put it in the README (see the arm patch).
> 
>>
>> I'm really not convinced this command buys us anything yet. I do agree that
>> we want automated testing - but can't we get that using QEMU and a
>> downloadable image file that we pass in as disk and have the distro boot do
>> its magic?
> 
> That seems very heavyweight as a sanity check, although I agree it is useful.

It's not really much more heavy weight. The "image file" could simply
contain your hello world binary. But with this we don't just verify
whether "bootefi" works, but also whether the default boot path works ok.

> Here I am just making sure that EFI programs can start, print output
> and exit. It is a test that we can easily run without a lot of
> overhead, much less than a full distro boot.

Again, I don't think it's much more overhead and I do believe it gives
us much cleaner separation between responsibilities of code (tests go
where tests are).


Alex

[U-Boot] [PATCH v3 2/8] efi: Use asmlinkage for EFIAPI

[Alexander Graf]

On 18/10/2016 22:37, Simon Glass wrote:
> Hi Alex,
> 
> On 18 October 2016 at 01:12, Alexander Graf <agraf@suse.de> wrote:
>> On 10/18/2016 04:29 AM, Simon Glass wrote:
>>>
>>> This is required for x86 and is also correct for ARM (since it is empty).
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>>
>>
>> (Replying here in lack for a cover letter)
>>
>> Could you please rebase your patches on top of
>>
>>   https://github.com/agraf/u-boot.git efi-next
>>
>> so that all the patches that I already queued are not repeated in the patch
>> set and we don't get any conflicts?
> 
> I can do that - but is this targeting -next? I was expecting these
> patches to land in master.

Sorry, they are on their way to master. It's just old habit wrt my
naming scheme:

  -next: current development branch, basically staging for master
  -release-number: future patches that I already applied proactively for
the next release, or backports :)

As it stands, I've only taken the base patches, not all of them. Would
you expect all of the x86 efi enablement to land in 2016.11?


Alex

[U-Boot] [PATCH v3 2/8] efi: Use asmlinkage for EFIAPI

[Simon Glass]

Hi Alex,

On 19 October 2016 at 01:11, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 18/10/2016 22:37, Simon Glass wrote:
>> Hi Alex,
>>
>> On 18 October 2016 at 01:12, Alexander Graf <agraf@suse.de> wrote:
>>> On 10/18/2016 04:29 AM, Simon Glass wrote:
>>>>
>>>> This is required for x86 and is also correct for ARM (since it is empty).
>>>>
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>>>
>>>
>>> (Replying here in lack for a cover letter)
>>>
>>> Could you please rebase your patches on top of
>>>
>>>   https://github.com/agraf/u-boot.git efi-next
>>>
>>> so that all the patches that I already queued are not repeated in the patch
>>> set and we don't get any conflicts?
>>
>> I can do that - but is this targeting -next? I was expecting these
>> patches to land in master.
>
> Sorry, they are on their way to master. It's just old habit wrt my
> naming scheme:
>
>   -next: current development branch, basically staging for master

I'd suggest calling that 'master'.

>   -release-number: future patches that I already applied proactively for
> the next release, or backports :)
>
> As it stands, I've only taken the base patches, not all of them. Would
> you expect all of the x86 efi enablement to land in 2016.11?

No, let's target the release after that.

Regards,
Simon

[U-Boot] [PATCH v3 7/8] x86: efi: Add a hello world test program

[Simon Glass]

Hi Alex,

On 19 October 2016 at 01:09, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 18/10/2016 22:37, Simon Glass wrote:
>> Hi Alex,
>>
>> On 18 October 2016 at 01:14, Alexander Graf <agraf@suse.de> wrote:
>>> On 10/18/2016 04:29 AM, Simon Glass wrote:
>>>>
>>>> It is useful to have a basic sanity check for EFI loader support. Add a
>>>> 'bootefi hello' command which loads HelloWord.efi and runs it under
>>>> U-Boot.
>>>>
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>> ---
>>>>
>>>> Changes in v3:
>>>> - Include a link to the program instead of adding it to the tree
>>>
>>>
>>> So, uh, where is the link?
>>
>> I put it in the README (see the arm patch).
>>
>>>
>>> I'm really not convinced this command buys us anything yet. I do agree that
>>> we want automated testing - but can't we get that using QEMU and a
>>> downloadable image file that we pass in as disk and have the distro boot do
>>> its magic?
>>
>> That seems very heavyweight as a sanity check, although I agree it is useful.
>
> It's not really much more heavy weight. The "image file" could simply
> contain your hello world binary. But with this we don't just verify
> whether "bootefi" works, but also whether the default boot path works ok.

I don't think I understand what you mean by 'image file'. Is it
something other than the .efi file? Do you mean a disk image?

>
>> Here I am just making sure that EFI programs can start, print output
>> and exit. It is a test that we can easily run without a lot of
>> overhead, much less than a full distro boot.
>
> Again, I don't think it's much more overhead and I do believe it gives
> us much cleaner separation between responsibilities of code (tests go
> where tests are).

You are talking about a functional test, something that tests things
end to end. I prefer to at least start with a smaller test. Granted it
takes a little more work but it means there are fewer things to hunt
through when something goes wrong.

Regards,
Simon

[U-Boot] [PATCH v3 7/8] x86: efi: Add a hello world test program

[Alexander Graf]

On 07/11/2016 10:46, Simon Glass wrote:
> Hi Alex,
>
> On 19 October 2016 at 01:09, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 18/10/2016 22:37, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 18 October 2016 at 01:14, Alexander Graf <agraf@suse.de> wrote:
>>>> On 10/18/2016 04:29 AM, Simon Glass wrote:
>>>>>
>>>>> It is useful to have a basic sanity check for EFI loader support. Add a
>>>>> 'bootefi hello' command which loads HelloWord.efi and runs it under
>>>>> U-Boot.
>>>>>
>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>> ---
>>>>>
>>>>> Changes in v3:
>>>>> - Include a link to the program instead of adding it to the tree
>>>>
>>>>
>>>> So, uh, where is the link?
>>>
>>> I put it in the README (see the arm patch).
>>>
>>>>
>>>> I'm really not convinced this command buys us anything yet. I do agree that
>>>> we want automated testing - but can't we get that using QEMU and a
>>>> downloadable image file that we pass in as disk and have the distro boot do
>>>> its magic?
>>>
>>> That seems very heavyweight as a sanity check, although I agree it is useful.
>>
>> It's not really much more heavy weight. The "image file" could simply
>> contain your hello world binary. But with this we don't just verify
>> whether "bootefi" works, but also whether the default boot path works ok.
>
> I don't think I understand what you mean by 'image file'. Is it
> something other than the .efi file? Do you mean a disk image?

Yes. For reasonable test coverage, we should also verify that the distro 
defaults wrote a sane boot script that automatically searches for a 
default EFI binary in /efi/boot/bootx86.efi on the first partition of 
all devices and runs it.

So if we just provide an SD card image or hard disk image to QEMU which 
contains a hello world .efi binary as that default boot file, we don't 
only test whether the "bootefi" command works, but also whether the 
distro boot script works.

>
>>
>>> Here I am just making sure that EFI programs can start, print output
>>> and exit. It is a test that we can easily run without a lot of
>>> overhead, much less than a full distro boot.
>>
>> Again, I don't think it's much more overhead and I do believe it gives
>> us much cleaner separation between responsibilities of code (tests go
>> where tests are).
>
> You are talking about a functional test, something that tests things
> end to end. I prefer to at least start with a smaller test. Granted it
> takes a little more work but it means there are fewer things to hunt
> through when something goes wrong.

Yes, I personally find unit tests terribly annoying and unproductive and 
functional tests very helpful :). And in this case, the effort to write 
it is about the same for both, just that the functional test actually 
tells you that things work or don't work at the end of the day.

With a code base like U-Boot, a simple functional test like the above 
plus git bisect should get you to an offending patch very quickly.


Alex

[U-Boot] [PATCH v3 7/8] x86: efi: Add a hello world test program

[Simon Glass]

Hi Alex,

On 7 November 2016 at 09:32, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 07/11/2016 10:46, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 19 October 2016 at 01:09, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>>
>>> On 18/10/2016 22:37, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On 18 October 2016 at 01:14, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>> On 10/18/2016 04:29 AM, Simon Glass wrote:
>>>>>>
>>>>>>
>>>>>> It is useful to have a basic sanity check for EFI loader support. Add
>>>>>> a
>>>>>> 'bootefi hello' command which loads HelloWord.efi and runs it under
>>>>>> U-Boot.
>>>>>>
>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>> ---
>>>>>>
>>>>>> Changes in v3:
>>>>>> - Include a link to the program instead of adding it to the tree
>>>>>
>>>>>
>>>>>
>>>>> So, uh, where is the link?
>>>>
>>>>
>>>> I put it in the README (see the arm patch).
>>>>
>>>>>
>>>>> I'm really not convinced this command buys us anything yet. I do agree
>>>>> that
>>>>> we want automated testing - but can't we get that using QEMU and a
>>>>> downloadable image file that we pass in as disk and have the distro
>>>>> boot do
>>>>> its magic?
>>>>
>>>>
>>>> That seems very heavyweight as a sanity check, although I agree it is
>>>> useful.
>>>
>>>
>>> It's not really much more heavy weight. The "image file" could simply
>>> contain your hello world binary. But with this we don't just verify
>>> whether "bootefi" works, but also whether the default boot path works ok.
>>
>>
>> I don't think I understand what you mean by 'image file'. Is it
>> something other than the .efi file? Do you mean a disk image?
>
>
> Yes. For reasonable test coverage, we should also verify that the distro
> defaults wrote a sane boot script that automatically searches for a default
> EFI binary in /efi/boot/bootx86.efi on the first partition of all devices
> and runs it.
>
> So if we just provide an SD card image or hard disk image to QEMU which
> contains a hello world .efi binary as that default boot file, we don't only
> test whether the "bootefi" command works, but also whether the distro boot
> script works.

That's right.

>
>>
>>>
>>>> Here I am just making sure that EFI programs can start, print output
>>>> and exit. It is a test that we can easily run without a lot of
>>>> overhead, much less than a full distro boot.
>>>
>>>
>>> Again, I don't think it's much more overhead and I do believe it gives
>>> us much cleaner separation between responsibilities of code (tests go
>>> where tests are).
>>
>>
>> You are talking about a functional test, something that tests things
>> end to end. I prefer to at least start with a smaller test. Granted it
>> takes a little more work but it means there are fewer things to hunt
>> through when something goes wrong.
>
>
> Yes, I personally find unit tests terribly annoying and unproductive and
> functional tests very helpful :). And in this case, the effort to write it
> is about the same for both, just that the functional test actually tells you
> that things work or don't work at the end of the day.
>
> With a code base like U-Boot, a simple functional test like the above plus
> git bisect should get you to an offending patch very quickly.

This is not a unit test - in fact the EFI stuff has no unit tests. I
suppose if we are trying to find a name this is a small functional
test since it exercises the general functionality.

I am much keener on small tests than large ones for finding simple
bugs. Of course you can generally bisect to find a bug, but the more
layers of software you need to look for the harder this is.

We could definitely use a pytest which checks an EFI boot into an
image, but I don't think this obviates the need for a smaller targeted
test like this one.

Regards,
Simon

[U-Boot] [PATCH v3 7/8] x86: efi: Add a hello world test program

[Alexander Graf]

> Am 11.11.2016 um 17:17 schrieb Simon Glass <sjg@chromium.org>:
> 
> Hi Alex,
> 
>> On 7 November 2016 at 09:32, Alexander Graf <agraf@suse.de> wrote:
>> 
>> 
>>> On 07/11/2016 10:46, Simon Glass wrote:
>>> 
>>> Hi Alex,
>>> 
>>>> On 19 October 2016 at 01:09, Alexander Graf <agraf@suse.de> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On 18/10/2016 22:37, Simon Glass wrote:
>>>>> 
>>>>> Hi Alex,
>>>>> 
>>>>>> On 18 October 2016 at 01:14, Alexander Graf <agraf@suse.de> wrote:
>>>>>> 
>>>>>>> On 10/18/2016 04:29 AM, Simon Glass wrote:
>>>>>>> 
>>>>>>> 
>>>>>>> It is useful to have a basic sanity check for EFI loader support. Add
>>>>>>> a
>>>>>>> 'bootefi hello' command which loads HelloWord.efi and runs it under
>>>>>>> U-Boot.
>>>>>>> 
>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>> ---
>>>>>>> 
>>>>>>> Changes in v3:
>>>>>>> - Include a link to the program instead of adding it to the tree
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> So, uh, where is the link?
>>>>> 
>>>>> 
>>>>> I put it in the README (see the arm patch).
>>>>> 
>>>>>> 
>>>>>> I'm really not convinced this command buys us anything yet. I do agree
>>>>>> that
>>>>>> we want automated testing - but can't we get that using QEMU and a
>>>>>> downloadable image file that we pass in as disk and have the distro
>>>>>> boot do
>>>>>> its magic?
>>>>> 
>>>>> 
>>>>> That seems very heavyweight as a sanity check, although I agree it is
>>>>> useful.
>>>> 
>>>> 
>>>> It's not really much more heavy weight. The "image file" could simply
>>>> contain your hello world binary. But with this we don't just verify
>>>> whether "bootefi" works, but also whether the default boot path works ok.
>>> 
>>> 
>>> I don't think I understand what you mean by 'image file'. Is it
>>> something other than the .efi file? Do you mean a disk image?
>> 
>> 
>> Yes. For reasonable test coverage, we should also verify that the distro
>> defaults wrote a sane boot script that automatically searches for a default
>> EFI binary in /efi/boot/bootx86.efi on the first partition of all devices
>> and runs it.
>> 
>> So if we just provide an SD card image or hard disk image to QEMU which
>> contains a hello world .efi binary as that default boot file, we don't only
>> test whether the "bootefi" command works, but also whether the distro boot
>> script works.
> 
> That's right.
> 
>> 
>>> 
>>>> 
>>>>> Here I am just making sure that EFI programs can start, print output
>>>>> and exit. It is a test that we can easily run without a lot of
>>>>> overhead, much less than a full distro boot.
>>>> 
>>>> 
>>>> Again, I don't think it's much more overhead and I do believe it gives
>>>> us much cleaner separation between responsibilities of code (tests go
>>>> where tests are).
>>> 
>>> 
>>> You are talking about a functional test, something that tests things
>>> end to end. I prefer to at least start with a smaller test. Granted it
>>> takes a little more work but it means there are fewer things to hunt
>>> through when something goes wrong.
>> 
>> 
>> Yes, I personally find unit tests terribly annoying and unproductive and
>> functional tests very helpful :). And in this case, the effort to write it
>> is about the same for both, just that the functional test actually tells you
>> that things work or don't work at the end of the day.
>> 
>> With a code base like U-Boot, a simple functional test like the above plus
>> git bisect should get you to an offending patch very quickly.
> 
> This is not a unit test - in fact the EFI stuff has no unit tests. I
> suppose if we are trying to find a name this is a small functional
> test since it exercises the general functionality.
> 
> I am much keener on small tests than large ones for finding simple
> bugs. Of course you can generally bisect to find a bug, but the more
> layers of software you need to look for the harder this is.
> 
> We could definitely use a pytest which checks an EFI boot into an
> image, but I don't think this obviates the need for a smaller targeted
> test like this one.

I think arguing over this is moot :). More tests is usually a good thing, so whoever gets to write them gets to push them ;). As long as the licenses are sound at least.


Alex

[U-Boot] [PATCH v3 7/8] x86: efi: Add a hello world test program

[Simon Glass]

Hi Alex,

On 11 November 2016 at 23:23, Alexander Graf <agraf@suse.de> wrote:
>
>
>> Am 11.11.2016 um 17:17 schrieb Simon Glass <sjg@chromium.org>:
>>
>> Hi Alex,
>>
>>> On 7 November 2016 at 09:32, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>>> On 07/11/2016 10:46, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>>> On 19 October 2016 at 01:09, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>>
>>>>>
>>>>>> On 18/10/2016 22:37, Simon Glass wrote:
>>>>>>
>>>>>> Hi Alex,
>>>>>>
>>>>>>> On 18 October 2016 at 01:14, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>
>>>>>>>> On 10/18/2016 04:29 AM, Simon Glass wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> It is useful to have a basic sanity check for EFI loader support. Add
>>>>>>>> a
>>>>>>>> 'bootefi hello' command which loads HelloWord.efi and runs it under
>>>>>>>> U-Boot.
>>>>>>>>
>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Changes in v3:
>>>>>>>> - Include a link to the program instead of adding it to the tree
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> So, uh, where is the link?
>>>>>>
>>>>>>
>>>>>> I put it in the README (see the arm patch).
>>>>>>
>>>>>>>
>>>>>>> I'm really not convinced this command buys us anything yet. I do agree
>>>>>>> that
>>>>>>> we want automated testing - but can't we get that using QEMU and a
>>>>>>> downloadable image file that we pass in as disk and have the distro
>>>>>>> boot do
>>>>>>> its magic?
>>>>>>
>>>>>>
>>>>>> That seems very heavyweight as a sanity check, although I agree it is
>>>>>> useful.
>>>>>
>>>>>
>>>>> It's not really much more heavy weight. The "image file" could simply
>>>>> contain your hello world binary. But with this we don't just verify
>>>>> whether "bootefi" works, but also whether the default boot path works ok.
>>>>
>>>>
>>>> I don't think I understand what you mean by 'image file'. Is it
>>>> something other than the .efi file? Do you mean a disk image?
>>>
>>>
>>> Yes. For reasonable test coverage, we should also verify that the distro
>>> defaults wrote a sane boot script that automatically searches for a default
>>> EFI binary in /efi/boot/bootx86.efi on the first partition of all devices
>>> and runs it.
>>>
>>> So if we just provide an SD card image or hard disk image to QEMU which
>>> contains a hello world .efi binary as that default boot file, we don't only
>>> test whether the "bootefi" command works, but also whether the distro boot
>>> script works.
>>
>> That's right.
>>
>>>
>>>>
>>>>>
>>>>>> Here I am just making sure that EFI programs can start, print output
>>>>>> and exit. It is a test that we can easily run without a lot of
>>>>>> overhead, much less than a full distro boot.
>>>>>
>>>>>
>>>>> Again, I don't think it's much more overhead and I do believe it gives
>>>>> us much cleaner separation between responsibilities of code (tests go
>>>>> where tests are).
>>>>
>>>>
>>>> You are talking about a functional test, something that tests things
>>>> end to end. I prefer to at least start with a smaller test. Granted it
>>>> takes a little more work but it means there are fewer things to hunt
>>>> through when something goes wrong.
>>>
>>>
>>> Yes, I personally find unit tests terribly annoying and unproductive and
>>> functional tests very helpful :). And in this case, the effort to write it
>>> is about the same for both, just that the functional test actually tells you
>>> that things work or don't work at the end of the day.
>>>
>>> With a code base like U-Boot, a simple functional test like the above plus
>>> git bisect should get you to an offending patch very quickly.
>>
>> This is not a unit test - in fact the EFI stuff has no unit tests. I
>> suppose if we are trying to find a name this is a small functional
>> test since it exercises the general functionality.
>>
>> I am much keener on small tests than large ones for finding simple
>> bugs. Of course you can generally bisect to find a bug, but the more
>> layers of software you need to look for the harder this is.
>>
>> We could definitely use a pytest which checks an EFI boot into an
>> image, but I don't think this obviates the need for a smaller targeted
>> test like this one.
>
> I think arguing over this is moot :). More tests is usually a good thing, so whoever gets to write them gets to push them ;). As long as the licenses are sound at least.

OK good, well please can you review this at some point? Also, are you
planning to write the 'larger' test? How do you test this all in suse?

Regards,
Simon

[U-Boot] [PATCH v3 7/8] x86: efi: Add a hello world test program

[Alexander Graf]

On 14/11/2016 21:44, Simon Glass wrote:
> Hi Alex,
>
> On 11 November 2016 at 23:23, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>>> Am 11.11.2016 um 17:17 schrieb Simon Glass <sjg@chromium.org>:
>>>
>>> Hi Alex,
>>>
>>>> On 7 November 2016 at 09:32, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>>
>>>>> On 07/11/2016 10:46, Simon Glass wrote:
>>>>>
>>>>> Hi Alex,
>>>>>
>>>>>> On 19 October 2016 at 01:09, Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>> On 18/10/2016 22:37, Simon Glass wrote:
>>>>>>>
>>>>>>> Hi Alex,
>>>>>>>
>>>>>>>> On 18 October 2016 at 01:14, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>
>>>>>>>>> On 10/18/2016 04:29 AM, Simon Glass wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> It is useful to have a basic sanity check for EFI loader support. Add
>>>>>>>>> a
>>>>>>>>> 'bootefi hello' command which loads HelloWord.efi and runs it under
>>>>>>>>> U-Boot.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> Changes in v3:
>>>>>>>>> - Include a link to the program instead of adding it to the tree
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> So, uh, where is the link?
>>>>>>>
>>>>>>>
>>>>>>> I put it in the README (see the arm patch).
>>>>>>>
>>>>>>>>
>>>>>>>> I'm really not convinced this command buys us anything yet. I do agree
>>>>>>>> that
>>>>>>>> we want automated testing - but can't we get that using QEMU and a
>>>>>>>> downloadable image file that we pass in as disk and have the distro
>>>>>>>> boot do
>>>>>>>> its magic?
>>>>>>>
>>>>>>>
>>>>>>> That seems very heavyweight as a sanity check, although I agree it is
>>>>>>> useful.
>>>>>>
>>>>>>
>>>>>> It's not really much more heavy weight. The "image file" could simply
>>>>>> contain your hello world binary. But with this we don't just verify
>>>>>> whether "bootefi" works, but also whether the default boot path works ok.
>>>>>
>>>>>
>>>>> I don't think I understand what you mean by 'image file'. Is it
>>>>> something other than the .efi file? Do you mean a disk image?
>>>>
>>>>
>>>> Yes. For reasonable test coverage, we should also verify that the distro
>>>> defaults wrote a sane boot script that automatically searches for a default
>>>> EFI binary in /efi/boot/bootx86.efi on the first partition of all devices
>>>> and runs it.
>>>>
>>>> So if we just provide an SD card image or hard disk image to QEMU which
>>>> contains a hello world .efi binary as that default boot file, we don't only
>>>> test whether the "bootefi" command works, but also whether the distro boot
>>>> script works.
>>>
>>> That's right.
>>>
>>>>
>>>>>
>>>>>>
>>>>>>> Here I am just making sure that EFI programs can start, print output
>>>>>>> and exit. It is a test that we can easily run without a lot of
>>>>>>> overhead, much less than a full distro boot.
>>>>>>
>>>>>>
>>>>>> Again, I don't think it's much more overhead and I do believe it gives
>>>>>> us much cleaner separation between responsibilities of code (tests go
>>>>>> where tests are).
>>>>>
>>>>>
>>>>> You are talking about a functional test, something that tests things
>>>>> end to end. I prefer to at least start with a smaller test. Granted it
>>>>> takes a little more work but it means there are fewer things to hunt
>>>>> through when something goes wrong.
>>>>
>>>>
>>>> Yes, I personally find unit tests terribly annoying and unproductive and
>>>> functional tests very helpful :). And in this case, the effort to write it
>>>> is about the same for both, just that the functional test actually tells you
>>>> that things work or don't work at the end of the day.
>>>>
>>>> With a code base like U-Boot, a simple functional test like the above plus
>>>> git bisect should get you to an offending patch very quickly.
>>>
>>> This is not a unit test - in fact the EFI stuff has no unit tests. I
>>> suppose if we are trying to find a name this is a small functional
>>> test since it exercises the general functionality.
>>>
>>> I am much keener on small tests than large ones for finding simple
>>> bugs. Of course you can generally bisect to find a bug, but the more
>>> layers of software you need to look for the harder this is.
>>>
>>> We could definitely use a pytest which checks an EFI boot into an
>>> image, but I don't think this obviates the need for a smaller targeted
>>> test like this one.
>>
>> I think arguing over this is moot :). More tests is usually a good thing, so whoever gets to write them gets to push them ;). As long as the licenses are sound at least.
>
> OK good, well please can you review this at some point?

Review what exactly?

> Also, are you
> planning to write the 'larger' test? How do you test this all in suse?

Planning yes, but I'm very good at not writing tests :).

Currently I'm testing this all in suse by running systems which rely on 
the code to work.


Alex

[U-Boot] [PATCH v3 7/8] x86: efi: Add a hello world test program

[Simon Glass]

Hi Alex,

On 14 November 2016 at 13:46, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 14/11/2016 21:44, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 11 November 2016 at 23:23, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>>
>>>> Am 11.11.2016 um 17:17 schrieb Simon Glass <sjg@chromium.org>:
>>>>
>>>> Hi Alex,
>>>>
>>>>> On 7 November 2016 at 09:32, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>>
>>>>>> On 07/11/2016 10:46, Simon Glass wrote:
>>>>>>
>>>>>> Hi Alex,
>>>>>>
>>>>>>> On 19 October 2016 at 01:09, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> On 18/10/2016 22:37, Simon Glass wrote:
>>>>>>>>
>>>>>>>> Hi Alex,
>>>>>>>>
>>>>>>>>> On 18 October 2016 at 01:14, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>
>>>>>>>>>> On 10/18/2016 04:29 AM, Simon Glass wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> It is useful to have a basic sanity check for EFI loader support.
>>>>>>>>>> Add
>>>>>>>>>> a
>>>>>>>>>> 'bootefi hello' command which loads HelloWord.efi and runs it
>>>>>>>>>> under
>>>>>>>>>> U-Boot.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>> Changes in v3:
>>>>>>>>>> - Include a link to the program instead of adding it to the tree
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> So, uh, where is the link?
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I put it in the README (see the arm patch).
>>>>>>>>
>>>>>>>>>
>>>>>>>>> I'm really not convinced this command buys us anything yet. I do
>>>>>>>>> agree
>>>>>>>>> that
>>>>>>>>> we want automated testing - but can't we get that using QEMU and a
>>>>>>>>> downloadable image file that we pass in as disk and have the distro
>>>>>>>>> boot do
>>>>>>>>> its magic?
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> That seems very heavyweight as a sanity check, although I agree it
>>>>>>>> is
>>>>>>>> useful.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> It's not really much more heavy weight. The "image file" could simply
>>>>>>> contain your hello world binary. But with this we don't just verify
>>>>>>> whether "bootefi" works, but also whether the default boot path works
>>>>>>> ok.
>>>>>>
>>>>>>
>>>>>>
>>>>>> I don't think I understand what you mean by 'image file'. Is it
>>>>>> something other than the .efi file? Do you mean a disk image?
>>>>>
>>>>>
>>>>>
>>>>> Yes. For reasonable test coverage, we should also verify that the
>>>>> distro
>>>>> defaults wrote a sane boot script that automatically searches for a
>>>>> default
>>>>> EFI binary in /efi/boot/bootx86.efi on the first partition of all
>>>>> devices
>>>>> and runs it.
>>>>>
>>>>> So if we just provide an SD card image or hard disk image to QEMU which
>>>>> contains a hello world .efi binary as that default boot file, we don't
>>>>> only
>>>>> test whether the "bootefi" command works, but also whether the distro
>>>>> boot
>>>>> script works.
>>>>
>>>>
>>>> That's right.
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> Here I am just making sure that EFI programs can start, print output
>>>>>>>> and exit. It is a test that we can easily run without a lot of
>>>>>>>> overhead, much less than a full distro boot.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Again, I don't think it's much more overhead and I do believe it
>>>>>>> gives
>>>>>>> us much cleaner separation between responsibilities of code (tests go
>>>>>>> where tests are).
>>>>>>
>>>>>>
>>>>>>
>>>>>> You are talking about a functional test, something that tests things
>>>>>> end to end. I prefer to at least start with a smaller test. Granted it
>>>>>> takes a little more work but it means there are fewer things to hunt
>>>>>> through when something goes wrong.
>>>>>
>>>>>
>>>>>
>>>>> Yes, I personally find unit tests terribly annoying and unproductive
>>>>> and
>>>>> functional tests very helpful :). And in this case, the effort to write
>>>>> it
>>>>> is about the same for both, just that the functional test actually
>>>>> tells you
>>>>> that things work or don't work at the end of the day.
>>>>>
>>>>> With a code base like U-Boot, a simple functional test like the above
>>>>> plus
>>>>> git bisect should get you to an offending patch very quickly.
>>>>
>>>>
>>>> This is not a unit test - in fact the EFI stuff has no unit tests. I
>>>> suppose if we are trying to find a name this is a small functional
>>>> test since it exercises the general functionality.
>>>>
>>>> I am much keener on small tests than large ones for finding simple
>>>> bugs. Of course you can generally bisect to find a bug, but the more
>>>> layers of software you need to look for the harder this is.
>>>>
>>>> We could definitely use a pytest which checks an EFI boot into an
>>>> image, but I don't think this obviates the need for a smaller targeted
>>>> test like this one.
>>>
>>>
>>> I think arguing over this is moot :). More tests is usually a good thing,
>>> so whoever gets to write them gets to push them ;). As long as the licenses
>>> are sound at least.
>>
>>
>> OK good, well please can you review this at some point?
>
>
> Review what exactly?

I mean the patches. There should be ~14 in your queue.

>
>> Also, are you
>> planning to write the 'larger' test? How do you test this all in suse?
>
>
> Planning yes, but I'm very good at not writing tests :).
>
> Currently I'm testing this all in suse by running systems which rely on the
> code to work.

OK I see.

Regards,
Simon

[U-Boot] [PATCH v3 7/8] x86: efi: Add a hello world test program

[Alexander Graf]

On 14/11/2016 21:58, Simon Glass wrote:
> Hi Alex,
>
> On 14 November 2016 at 13:46, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 14/11/2016 21:44, Simon Glass wrote:
>>>
>>> Hi Alex,
>>>
>>> On 11 November 2016 at 23:23, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>>
>>>>
>>>>> Am 11.11.2016 um 17:17 schrieb Simon Glass <sjg@chromium.org>:
>>>>>
>>>>> Hi Alex,
>>>>>
>>>>>> On 7 November 2016 at 09:32, Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>>
>>>>>>> On 07/11/2016 10:46, Simon Glass wrote:
>>>>>>>
>>>>>>> Hi Alex,
>>>>>>>
>>>>>>>> On 19 October 2016 at 01:09, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> On 18/10/2016 22:37, Simon Glass wrote:
>>>>>>>>>
>>>>>>>>> Hi Alex,
>>>>>>>>>
>>>>>>>>>> On 18 October 2016 at 01:14, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>>
>>>>>>>>>>> On 10/18/2016 04:29 AM, Simon Glass wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> It is useful to have a basic sanity check for EFI loader support.
>>>>>>>>>>> Add
>>>>>>>>>>> a
>>>>>>>>>>> 'bootefi hello' command which loads HelloWord.efi and runs it
>>>>>>>>>>> under
>>>>>>>>>>> U-Boot.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>>>>>> ---
>>>>>>>>>>>
>>>>>>>>>>> Changes in v3:
>>>>>>>>>>> - Include a link to the program instead of adding it to the tree
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> So, uh, where is the link?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I put it in the README (see the arm patch).
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I'm really not convinced this command buys us anything yet. I do
>>>>>>>>>> agree
>>>>>>>>>> that
>>>>>>>>>> we want automated testing - but can't we get that using QEMU and a
>>>>>>>>>> downloadable image file that we pass in as disk and have the distro
>>>>>>>>>> boot do
>>>>>>>>>> its magic?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> That seems very heavyweight as a sanity check, although I agree it
>>>>>>>>> is
>>>>>>>>> useful.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> It's not really much more heavy weight. The "image file" could simply
>>>>>>>> contain your hello world binary. But with this we don't just verify
>>>>>>>> whether "bootefi" works, but also whether the default boot path works
>>>>>>>> ok.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I don't think I understand what you mean by 'image file'. Is it
>>>>>>> something other than the .efi file? Do you mean a disk image?
>>>>>>
>>>>>>
>>>>>>
>>>>>> Yes. For reasonable test coverage, we should also verify that the
>>>>>> distro
>>>>>> defaults wrote a sane boot script that automatically searches for a
>>>>>> default
>>>>>> EFI binary in /efi/boot/bootx86.efi on the first partition of all
>>>>>> devices
>>>>>> and runs it.
>>>>>>
>>>>>> So if we just provide an SD card image or hard disk image to QEMU which
>>>>>> contains a hello world .efi binary as that default boot file, we don't
>>>>>> only
>>>>>> test whether the "bootefi" command works, but also whether the distro
>>>>>> boot
>>>>>> script works.
>>>>>
>>>>>
>>>>> That's right.
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> Here I am just making sure that EFI programs can start, print output
>>>>>>>>> and exit. It is a test that we can easily run without a lot of
>>>>>>>>> overhead, much less than a full distro boot.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Again, I don't think it's much more overhead and I do believe it
>>>>>>>> gives
>>>>>>>> us much cleaner separation between responsibilities of code (tests go
>>>>>>>> where tests are).
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> You are talking about a functional test, something that tests things
>>>>>>> end to end. I prefer to at least start with a smaller test. Granted it
>>>>>>> takes a little more work but it means there are fewer things to hunt
>>>>>>> through when something goes wrong.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Yes, I personally find unit tests terribly annoying and unproductive
>>>>>> and
>>>>>> functional tests very helpful :). And in this case, the effort to write
>>>>>> it
>>>>>> is about the same for both, just that the functional test actually
>>>>>> tells you
>>>>>> that things work or don't work at the end of the day.
>>>>>>
>>>>>> With a code base like U-Boot, a simple functional test like the above
>>>>>> plus
>>>>>> git bisect should get you to an offending patch very quickly.
>>>>>
>>>>>
>>>>> This is not a unit test - in fact the EFI stuff has no unit tests. I
>>>>> suppose if we are trying to find a name this is a small functional
>>>>> test since it exercises the general functionality.
>>>>>
>>>>> I am much keener on small tests than large ones for finding simple
>>>>> bugs. Of course you can generally bisect to find a bug, but the more
>>>>> layers of software you need to look for the harder this is.
>>>>>
>>>>> We could definitely use a pytest which checks an EFI boot into an
>>>>> image, but I don't think this obviates the need for a smaller targeted
>>>>> test like this one.
>>>>
>>>>
>>>> I think arguing over this is moot :). More tests is usually a good thing,
>>>> so whoever gets to write them gets to push them ;). As long as the licenses
>>>> are sound at least.
>>>
>>>
>>> OK good, well please can you review this at some point?
>>
>>
>> Review what exactly?
>
> I mean the patches. There should be ~14 in your queue.

Interesting. I see them on patchwork, but neither in my U-Boot folder, 
my inbox nor my spam folder. I wonder what happened there.


Alex

[U-Boot] [PATCH v3 7/8] x86: efi: Add a hello world test program

[Simon Glass]

Hi Alex,

On 14 November 2016 at 14:24, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 14/11/2016 21:58, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 14 November 2016 at 13:46, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>>
>>> On 14/11/2016 21:44, Simon Glass wrote:
>>>>
>>>>
>>>> Hi Alex,
>>>>
>>>> On 11 November 2016 at 23:23, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>> Am 11.11.2016 um 17:17 schrieb Simon Glass <sjg@chromium.org>:
>>>>>>
>>>>>> Hi Alex,
>>>>>>
>>>>>>> On 7 November 2016 at 09:32, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>
>>>>>>>
>>>>>>>> On 07/11/2016 10:46, Simon Glass wrote:
>>>>>>>>
>>>>>>>> Hi Alex,
>>>>>>>>
>>>>>>>>> On 19 October 2016 at 01:09, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> On 18/10/2016 22:37, Simon Glass wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Alex,
>>>>>>>>>>
>>>>>>>>>>> On 18 October 2016 at 01:14, Alexander Graf <agraf@suse.de>
>>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> On 10/18/2016 04:29 AM, Simon Glass wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> It is useful to have a basic sanity check for EFI loader
>>>>>>>>>>>> support.
>>>>>>>>>>>> Add
>>>>>>>>>>>> a
>>>>>>>>>>>> 'bootefi hello' command which loads HelloWord.efi and runs it
>>>>>>>>>>>> under
>>>>>>>>>>>> U-Boot.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>>>>>>>>>> Changes in v3:
>>>>>>>>>>>> - Include a link to the program instead of adding it to the tree
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> So, uh, where is the link?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I put it in the README (see the arm patch).
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I'm really not convinced this command buys us anything yet. I do
>>>>>>>>>>> agree
>>>>>>>>>>> that
>>>>>>>>>>> we want automated testing - but can't we get that using QEMU and
>>>>>>>>>>> a
>>>>>>>>>>> downloadable image file that we pass in as disk and have the
>>>>>>>>>>> distro
>>>>>>>>>>> boot do
>>>>>>>>>>> its magic?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> That seems very heavyweight as a sanity check, although I agree it
>>>>>>>>>> is
>>>>>>>>>> useful.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> It's not really much more heavy weight. The "image file" could
>>>>>>>>> simply
>>>>>>>>> contain your hello world binary. But with this we don't just verify
>>>>>>>>> whether "bootefi" works, but also whether the default boot path
>>>>>>>>> works
>>>>>>>>> ok.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I don't think I understand what you mean by 'image file'. Is it
>>>>>>>> something other than the .efi file? Do you mean a disk image?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Yes. For reasonable test coverage, we should also verify that the
>>>>>>> distro
>>>>>>> defaults wrote a sane boot script that automatically searches for a
>>>>>>> default
>>>>>>> EFI binary in /efi/boot/bootx86.efi on the first partition of all
>>>>>>> devices
>>>>>>> and runs it.
>>>>>>>
>>>>>>> So if we just provide an SD card image or hard disk image to QEMU
>>>>>>> which
>>>>>>> contains a hello world .efi binary as that default boot file, we
>>>>>>> don't
>>>>>>> only
>>>>>>> test whether the "bootefi" command works, but also whether the distro
>>>>>>> boot
>>>>>>> script works.
>>>>>>
>>>>>>
>>>>>>
>>>>>> That's right.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Here I am just making sure that EFI programs can start, print
>>>>>>>>>> output
>>>>>>>>>> and exit. It is a test that we can easily run without a lot of
>>>>>>>>>> overhead, much less than a full distro boot.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Again, I don't think it's much more overhead and I do believe it
>>>>>>>>> gives
>>>>>>>>> us much cleaner separation between responsibilities of code (tests
>>>>>>>>> go
>>>>>>>>> where tests are).
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> You are talking about a functional test, something that tests things
>>>>>>>> end to end. I prefer to at least start with a smaller test. Granted
>>>>>>>> it
>>>>>>>> takes a little more work but it means there are fewer things to hunt
>>>>>>>> through when something goes wrong.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Yes, I personally find unit tests terribly annoying and unproductive
>>>>>>> and
>>>>>>> functional tests very helpful :). And in this case, the effort to
>>>>>>> write
>>>>>>> it
>>>>>>> is about the same for both, just that the functional test actually
>>>>>>> tells you
>>>>>>> that things work or don't work at the end of the day.
>>>>>>>
>>>>>>> With a code base like U-Boot, a simple functional test like the above
>>>>>>> plus
>>>>>>> git bisect should get you to an offending patch very quickly.
>>>>>>
>>>>>>
>>>>>>
>>>>>> This is not a unit test - in fact the EFI stuff has no unit tests. I
>>>>>> suppose if we are trying to find a name this is a small functional
>>>>>> test since it exercises the general functionality.
>>>>>>
>>>>>> I am much keener on small tests than large ones for finding simple
>>>>>> bugs. Of course you can generally bisect to find a bug, but the more
>>>>>> layers of software you need to look for the harder this is.
>>>>>>
>>>>>> We could definitely use a pytest which checks an EFI boot into an
>>>>>> image, but I don't think this obviates the need for a smaller targeted
>>>>>> test like this one.
>>>>>
>>>>>
>>>>>
>>>>> I think arguing over this is moot :). More tests is usually a good
>>>>> thing,
>>>>> so whoever gets to write them gets to push them ;). As long as the
>>>>> licenses
>>>>> are sound at least.
>>>>
>>>>
>>>>
>>>> OK good, well please can you review this at some point?
>>>
>>>
>>>
>>> Review what exactly?
>>
>>
>> I mean the patches. There should be ~14 in your queue.
>
>
> Interesting. I see them on patchwork, but neither in my U-Boot folder, my
> inbox nor my spam folder. I wonder what happened there.

I'm really not sure but I have seen similar things. I will see if I
can figure it out.

Regards,
Simon