[U-Boot] [PATCH v2 00/11] efi_loader: error handling cmd/bootefi.c

[U-Boot] [PATCH v2 00/11] efi_loader: error handling cmd/bootefi.c

[Heinrich Schuchardt]

This patch series is focused on providing error checking during the
initialization of the EFI subsystem.

If any of the registrations routines fails bootefi is not executed.
As currently the registration attempt cannot be unroled any further
bootefi call will fail.

Passing a devicetree to bootefi selftest is enabled and a unit test is
supplied.

Heinrich Schuchardt (11):
  efi_loader: efi_smbios_register should have a return value
  efi_loader: return efi_status_t from efi_gop_register
  efi_loader: return efi_status_t from efi_net_register
  efi_loader: consistently return efi_status_t efi_watchdog_register
  efi_loader: simplify calling efi_init_obj_list
  efi_loader: exit status for efi_reset_system_init
  efi_loader: efi_get_time_init should return status code
  efi_loader: do_bootefi_exec should always return an EFI status code
  efi_loader: check initialization of EFI subsystem is successful
  efi_loader: support device tree for bootefi selftest
  efi_selftest: check installation of the device tree

 arch/arm/cpu/armv8/fsl-layerscape/cpu.c |   4 +-
 arch/arm/mach-bcm283x/reset.c           |   4 +-
 cmd/bootefi.c                           | 184 +++++++++++++++++++------------
 include/efi_loader.h                    |  23 ++--
 lib/efi_loader/efi_boottime.c           |   2 +
 lib/efi_loader/efi_gop.c                |  34 ++++--
 lib/efi_loader/efi_net.c                |  24 ++--
 lib/efi_loader/efi_runtime.c            |  18 ++-
 lib/efi_loader/efi_smbios.c             |  23 ++--
 lib/efi_loader/efi_watchdog.c           |   4 +-
 lib/efi_selftest/Makefile               |   1 +
 lib/efi_selftest/efi_selftest_fdt.c     | 188 ++++++++++++++++++++++++++++++++
 test/py/tests/test_efi_selftest.py      |  14 +++
 13 files changed, 404 insertions(+), 119 deletions(-)
 create mode 100644 lib/efi_selftest/efi_selftest_fdt.c

-- 
2.15.1

[U-Boot] [PATCH v2 01/11] efi_loader: efi_smbios_register should have a return value

[Heinrich Schuchardt]

Errors may occur inside efi_smbios_register().

- Return a status code.
- Remove unused variables.
- Use constants where applicable.

Suggested-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	remove a change in unrelated code
	resent with this patch series
---
 include/efi_loader.h        |  2 +-
 lib/efi_loader/efi_smbios.c | 23 +++++++++++++++--------
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 07730c3f394..72c83fd5033 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -185,7 +185,7 @@ int efi_net_register(void);
 /* Called by bootefi to make the watchdog available */
 int efi_watchdog_register(void);
 /* Called by bootefi to make SMBIOS tables available */
-void efi_smbios_register(void);
+efi_status_t efi_smbios_register(void);
 
 struct efi_simple_file_system_protocol *
 efi_fs_from_path(struct efi_device_path *fp);
diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c
index ac412e7362a..62e96979021 100644
--- a/lib/efi_loader/efi_smbios.c
+++ b/lib/efi_loader/efi_smbios.c
@@ -13,20 +13,27 @@
 
 static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID;
 
-void efi_smbios_register(void)
+/*
+ * Install the SMBIOS table as a configuration table.
+ *
+ * @return	status code
+ */
+efi_status_t efi_smbios_register(void)
 {
 	/* Map within the low 32 bits, to allow for 32bit SMBIOS tables */
-	uint64_t dmi = 0xffffffff;
-	/* Reserve 4kb for SMBIOS */
-	uint64_t pages = 1;
-	int memtype = EFI_RUNTIME_SERVICES_DATA;
+	u64 dmi = U32_MAX;
+	efi_status_t ret;
 
-	if (efi_allocate_pages(1, memtype, pages, &dmi) != EFI_SUCCESS)
-		return;
+	/* Reserve 4kiB page for SMBIOS */
+	ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
+				 EFI_RUNTIME_SERVICES_DATA, 1, &dmi);
+	if (ret != EFI_SUCCESS)
+		return ret;
 
 	/* Generate SMBIOS tables */
 	write_smbios_table(dmi);
 
 	/* And expose them to our EFI payload */
-	efi_install_configuration_table(&smbios_guid, (void*)(uintptr_t)dmi);
+	return efi_install_configuration_table(&smbios_guid,
+					       (void *)(uintptr_t)dmi);
 }
-- 
2.15.1

[U-Boot] [PATCH v2 02/11] efi_loader: return efi_status_t from efi_gop_register

[Heinrich Schuchardt]

All initialization routines should return a status code instead of
a boolean.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	new patch
---
 include/efi_loader.h     |  2 +-
 lib/efi_loader/efi_gop.c | 34 ++++++++++++++++++++++------------
 2 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 72c83fd5033..779b8bde2e3 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -179,7 +179,7 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
 			       const char *if_typename, int diskid,
 			       const char *pdevname);
 /* Called by bootefi to make GOP (graphical) interface available */
-int efi_gop_register(void);
+efi_status_t efi_gop_register(void);
 /* Called by bootefi to make the network interface available */
 int efi_net_register(void);
 /* Called by bootefi to make the watchdog available */
diff --git a/lib/efi_loader/efi_gop.c b/lib/efi_loader/efi_gop.c
index 3caddd5f844..91b0b6a064b 100644
--- a/lib/efi_loader/efi_gop.c
+++ b/lib/efi_loader/efi_gop.c
@@ -125,8 +125,13 @@ efi_status_t EFIAPI gop_blt(struct efi_gop *this, void *buffer,
 	return EFI_EXIT(EFI_SUCCESS);
 }
 
-/* This gets called from do_bootefi_exec(). */
-int efi_gop_register(void)
+/*
+ * Install graphical output protocol.
+ *
+ * If no supported video device exists this is not considered as an
+ * error.
+ */
+efi_status_t efi_gop_register(void)
 {
 	struct efi_gop_obj *gopobj;
 	u32 bpix, col, row;
@@ -136,12 +141,15 @@ int efi_gop_register(void)
 
 #ifdef CONFIG_DM_VIDEO
 	struct udevice *vdev;
+	struct video_priv *priv;
 
 	/* We only support a single video output device for now */
-	if (uclass_first_device(UCLASS_VIDEO, &vdev) || !vdev)
-		return -1;
+	if (uclass_first_device(UCLASS_VIDEO, &vdev) || !vdev) {
+		printf("WARNING: No video device\n");
+		return EFI_SUCCESS;
+	}
 
-	struct video_priv *priv = dev_get_uclass_priv(vdev);
+	priv = dev_get_uclass_priv(vdev);
 	bpix = priv->bpix;
 	col = video_get_xsize(vdev);
 	row = video_get_ysize(vdev);
@@ -170,13 +178,14 @@ int efi_gop_register(void)
 		break;
 	default:
 		/* So far, we only work in 16 or 32 bit mode */
-		return -1;
+		printf("WARNING: Unsupported video mode\n");
+		return EFI_SUCCESS;
 	}
 
 	gopobj = calloc(1, sizeof(*gopobj));
 	if (!gopobj) {
 		printf("ERROR: Out of memory\n");
-		return 1;
+		return EFI_OUT_OF_RESOURCES;
 	}
 
 	/* Hook up to the device list */
@@ -186,8 +195,8 @@ int efi_gop_register(void)
 	ret = efi_add_protocol(gopobj->parent.handle, &efi_gop_guid,
 			       &gopobj->ops);
 	if (ret != EFI_SUCCESS) {
-		printf("ERROR: Out of memory\n");
-		return 1;
+		printf("ERROR: Failure adding gop protocol\n");
+		return ret;
 	}
 	gopobj->ops.query_mode = gop_query_mode;
 	gopobj->ops.set_mode = gop_set_mode;
@@ -199,10 +208,11 @@ int efi_gop_register(void)
 	gopobj->mode.info_size = sizeof(gopobj->info);
 
 #ifdef CONFIG_DM_VIDEO
-	if (bpix == VIDEO_BPP32) {
+	if (bpix == VIDEO_BPP32)
 #else
-	if (bpix == LCD_COLOR32) {
+	if (bpix == LCD_COLOR32)
 #endif
+	{
 		/* With 32bit color space we can directly expose the fb */
 		gopobj->mode.fb_base = fb_base;
 		gopobj->mode.fb_size = fb_size;
@@ -217,5 +227,5 @@ int efi_gop_register(void)
 	gopobj->bpix = bpix;
 	gopobj->fb = fb;
 
-	return 0;
+	return EFI_SUCCESS;
 }
-- 
2.15.1

[U-Boot] [PATCH v2 03/11] efi_loader: return efi_status_t from efi_net_register

[Heinrich Schuchardt]

Consistently return status codes form efi_net_register().

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	new patch
---
 include/efi_loader.h     |  2 +-
 lib/efi_loader/efi_net.c | 24 +++++++++++++-----------
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 779b8bde2e3..dd6ffdc4843 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -181,7 +181,7 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
 /* Called by bootefi to make GOP (graphical) interface available */
 efi_status_t efi_gop_register(void);
 /* Called by bootefi to make the network interface available */
-int efi_net_register(void);
+efi_status_t efi_net_register(void);
 /* Called by bootefi to make the watchdog available */
 int efi_watchdog_register(void);
 /* Called by bootefi to make SMBIOS tables available */
diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
index 8c5d5b492ca..e7fed794502 100644
--- a/lib/efi_loader/efi_net.c
+++ b/lib/efi_loader/efi_net.c
@@ -280,20 +280,22 @@ static void EFIAPI efi_network_timer_notify(struct efi_event *event,
 }
 
 /* This gets called from do_bootefi_exec(). */
-int efi_net_register(void)
+efi_status_t efi_net_register(void)
 {
 	struct efi_net_obj *netobj;
 	efi_status_t r;
 
 	if (!eth_get_dev()) {
 		/* No eth device active, don't expose any */
-		return 0;
+		return EFI_SUCCESS;
 	}
 
 	/* We only expose the "active" eth device, so one is enough */
 	netobj = calloc(1, sizeof(*netobj));
-	if (!netobj)
-		goto out_of_memory;
+	if (!netobj) {
+		printf("ERROR: Out of memory\n");
+		return EFI_OUT_OF_RESOURCES;
+	}
 
 	/* Hook net up to the device list */
 	efi_add_handle(&netobj->parent);
@@ -302,15 +304,15 @@ int efi_net_register(void)
 	r = efi_add_protocol(netobj->parent.handle, &efi_net_guid,
 			     &netobj->net);
 	if (r != EFI_SUCCESS)
-		goto out_of_memory;
+		goto failure_to_add_protocol;
 	r = efi_add_protocol(netobj->parent.handle, &efi_guid_device_path,
 			     efi_dp_from_eth());
 	if (r != EFI_SUCCESS)
-		goto out_of_memory;
+		goto failure_to_add_protocol;
 	r = efi_add_protocol(netobj->parent.handle, &efi_pxe_guid,
 			     &netobj->pxe);
 	if (r != EFI_SUCCESS)
-		goto out_of_memory;
+		goto failure_to_add_protocol;
 	netobj->net.revision = EFI_SIMPLE_NETWORK_PROTOCOL_REVISION;
 	netobj->net.start = efi_net_start;
 	netobj->net.stop = efi_net_stop;
@@ -366,8 +368,8 @@ int efi_net_register(void)
 		return r;
 	}
 
-	return 0;
-out_of_memory:
-	printf("ERROR: Out of memory\n");
-	return 1;
+	return EFI_SUCCESS;
+failure_to_add_protocol:
+	printf("ERROR: Failure to add protocol\n");
+	return r;
 }
-- 
2.15.1

[U-Boot] [PATCH v2 04/11] efi_loader: consistently return efi_status_t efi_watchdog_register

[Heinrich Schuchardt]

efi_watchdog_register() should always return a status code and not
a boolean value.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	new patch
---
 include/efi_loader.h          | 2 +-
 lib/efi_loader/efi_watchdog.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index dd6ffdc4843..08e6c6fb9c6 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -183,7 +183,7 @@ efi_status_t efi_gop_register(void);
 /* Called by bootefi to make the network interface available */
 efi_status_t efi_net_register(void);
 /* Called by bootefi to make the watchdog available */
-int efi_watchdog_register(void);
+efi_status_t efi_watchdog_register(void);
 /* Called by bootefi to make SMBIOS tables available */
 efi_status_t efi_smbios_register(void);
 
diff --git a/lib/efi_loader/efi_watchdog.c b/lib/efi_loader/efi_watchdog.c
index 35a45dedf84..b1c35a8e29d 100644
--- a/lib/efi_loader/efi_watchdog.c
+++ b/lib/efi_loader/efi_watchdog.c
@@ -59,7 +59,7 @@ efi_status_t efi_set_watchdog(unsigned long timeout)
  *
  * This function is called by efi_init_obj_list()
  */
-int efi_watchdog_register(void)
+efi_status_t efi_watchdog_register(void)
 {
 	efi_status_t r;
 
@@ -85,5 +85,5 @@ int efi_watchdog_register(void)
 		printf("ERROR: Failed to set watchdog timer\n");
 		return r;
 	}
-	return 0;
+	return EFI_SUCCESS;
 }
-- 
2.15.1

[U-Boot] [PATCH v2 05/11] efi_loader: simplify calling efi_init_obj_list

[Heinrich Schuchardt]

efi_init_obj_list() should be executed only once.

Rather than having the caller check this variable and the callee set it,
move all access to the variable inside the function. This reduces the
logic needed to call efi_init_obj_list().

Suggested-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	no change, patch resent
---
 cmd/bootefi.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 2106ed9c8c8..dff86cf9f9a 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -22,7 +22,7 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-static uint8_t efi_obj_list_initalized;
+static u8 efi_obj_list_initialized;
 
 static struct efi_device_path *bootefi_image_path;
 static struct efi_device_path *bootefi_device_path;
@@ -30,7 +30,10 @@ static struct efi_device_path *bootefi_device_path;
 /* Initialize and populate EFI object list */
 static void efi_init_obj_list(void)
 {
-	efi_obj_list_initalized = 1;
+	/* Initialize once only */
+	if (efi_obj_list_initialized)
+		return;
+	efi_obj_list_initialized = 1;
 
 	/* Initialize EFI driver uclass */
 	efi_driver_init();
@@ -184,8 +187,7 @@ static efi_status_t do_bootefi_exec(void *efi, void *fdt,
 	}
 
 	/* Initialize and populate EFI object list */
-	if (!efi_obj_list_initalized)
-		efi_init_obj_list();
+	efi_init_obj_list();
 
 	efi_setup_loaded_image(&loaded_image_info, &loaded_image_info_obj,
 			       device_path, image_path);
@@ -284,8 +286,7 @@ static int do_bootefi_bootmgr_exec(unsigned long fdt_addr)
 	efi_status_t r;
 
 	/* Initialize and populate EFI object list */
-	if (!efi_obj_list_initalized)
-		efi_init_obj_list();
+	efi_init_obj_list();
 
 	/*
 	 * gd lives in a fixed register which may get clobbered while we execute
@@ -350,8 +351,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		 */
 		efi_save_gd();
 		/* Initialize and populate EFI object list */
-		if (!efi_obj_list_initalized)
-			efi_init_obj_list();
+		efi_init_obj_list();
 		/* Transfer environment variable efi_selftest as load options */
 		set_load_options(&loaded_image_info, "efi_selftest");
 		/* Execute the test */
-- 
2.15.1

[U-Boot] [PATCH v2 06/11] efi_loader: exit status for efi_reset_system_init

[Heinrich Schuchardt]

efi_reset_system_init provides the architecture or board specific
initialization of the EFI subsystem. Errors should be caught and
signalled by a return code.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	new patch
---
 arch/arm/cpu/armv8/fsl-layerscape/cpu.c |  4 ++--
 arch/arm/mach-bcm283x/reset.c           |  4 ++--
 include/efi_loader.h                    | 11 ++++++++---
 lib/efi_loader/efi_runtime.c            | 15 ++++++++++++---
 4 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/arch/arm/cpu/armv8/fsl-layerscape/cpu.c b/arch/arm/cpu/armv8/fsl-layerscape/cpu.c
index 70a60709357..702c6b69565 100644
--- a/arch/arm/cpu/armv8/fsl-layerscape/cpu.c
+++ b/arch/arm/cpu/armv8/fsl-layerscape/cpu.c
@@ -654,9 +654,9 @@ void __efi_runtime EFIAPI efi_reset_system(
 	while (1) { }
 }
 
-void efi_reset_system_init(void)
+efi_status_t efi_reset_system_init(void)
 {
-       efi_add_runtime_mmio(&rstcr, sizeof(*rstcr));
+	return efi_add_runtime_mmio(&rstcr, sizeof(*rstcr));
 }
 
 #endif
diff --git a/arch/arm/mach-bcm283x/reset.c b/arch/arm/mach-bcm283x/reset.c
index b62cb8a51ee..5b83fdf43d6 100644
--- a/arch/arm/mach-bcm283x/reset.c
+++ b/arch/arm/mach-bcm283x/reset.c
@@ -82,9 +82,9 @@ void __efi_runtime EFIAPI efi_reset_system(
 	while (1) { }
 }
 
-void efi_reset_system_init(void)
+efi_status_t efi_reset_system_init(void)
 {
-	efi_add_runtime_mmio(&wdog_regs, sizeof(*wdog_regs));
+	return efi_add_runtime_mmio(&wdog_regs, sizeof(*wdog_regs));
 }
 
 #endif
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 08e6c6fb9c6..199077d295d 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -346,7 +346,7 @@ static inline int guidcmp(const efi_guid_t *g1, const efi_guid_t *g2)
 
 /* Call this with mmio_ptr as the _pointer_ to a pointer to an MMIO region
  * to make it available at runtime */
-void efi_add_runtime_mmio(void *mmio_ptr, u64 len);
+efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len);
 
 /* Boards may provide the functions below to implement RTS functionality */
 
@@ -354,7 +354,9 @@ void __efi_runtime EFIAPI efi_reset_system(
 			enum efi_reset_type reset_type,
 			efi_status_t reset_status,
 			unsigned long data_size, void *reset_data);
-void efi_reset_system_init(void);
+
+/* Architecture specific initialization of the EFI subsystem */
+efi_status_t efi_reset_system_init(void);
 
 efi_status_t __efi_runtime EFIAPI efi_get_time(
 			struct efi_time *time,
@@ -388,7 +390,10 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
 /* Without CONFIG_EFI_LOADER we don't have a runtime section, stub it out */
 #define __efi_runtime_data
 #define __efi_runtime
-static inline void efi_add_runtime_mmio(void *mmio_ptr, u64 len) { }
+static inline efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len)
+{
+	return EFI_SUCCESS;
+}
 
 /* No loader configured, stub out EFI_ENTRY */
 static inline void efi_restore_gd(void) { }
diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index ccb4fc6141b..85f85711ddc 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -134,8 +134,9 @@ void __weak __efi_runtime EFIAPI efi_reset_system(
 	while (1) { }
 }
 
-void __weak efi_reset_system_init(void)
+efi_status_t __weak efi_reset_system_init(void)
 {
+	return EFI_SUCCESS;
 }
 
 efi_status_t __weak __efi_runtime EFIAPI efi_get_time(
@@ -332,18 +333,26 @@ static efi_status_t EFIAPI efi_set_virtual_address_map(
 	return EFI_EXIT(EFI_INVALID_PARAMETER);
 }
 
-void efi_add_runtime_mmio(void *mmio_ptr, u64 len)
+efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len)
 {
 	struct efi_runtime_mmio_list *newmmio;
+	efi_status_t ret;
 
 	u64 pages = (len + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
-	efi_add_memory_map(*(uintptr_t *)mmio_ptr, pages, EFI_MMAP_IO, false);
+	ret = efi_add_memory_map(*(uintptr_t *)mmio_ptr, pages, EFI_MMAP_IO,
+				 false);
+	if (ret != EFI_SUCCESS)
+		return ret;
 
 	newmmio = calloc(1, sizeof(*newmmio));
+	if (!newmmio)
+		return EFI_OUT_OF_RESOURCES;
 	newmmio->ptr = mmio_ptr;
 	newmmio->paddr = *(uintptr_t *)mmio_ptr;
 	newmmio->len = len;
 	list_add_tail(&newmmio->link, &efi_runtime_mmio);
+
+	return ret;
 }
 
 /*
-- 
2.15.1

[U-Boot] [PATCH v2 07/11] efi_loader: efi_get_time_init should return status code

[Heinrich Schuchardt]

All EFI initialization functions should return a status code.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	new patch
---
 include/efi_loader.h         | 2 +-
 lib/efi_loader/efi_runtime.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 199077d295d..85a47ca2f5e 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -361,7 +361,7 @@ efi_status_t efi_reset_system_init(void);
 efi_status_t __efi_runtime EFIAPI efi_get_time(
 			struct efi_time *time,
 			struct efi_time_cap *capabilities);
-void efi_get_time_init(void);
+efi_status_t efi_get_time_init(void);
 
 #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
 /*
diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 85f85711ddc..c59d161c8fe 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -147,8 +147,9 @@ efi_status_t __weak __efi_runtime EFIAPI efi_get_time(
 	return EFI_DEVICE_ERROR;
 }
 
-void __weak efi_get_time_init(void)
+efi_status_t __weak efi_get_time_init(void)
 {
+	return EFI_SUCCESS;
 }
 
 struct efi_runtime_detach_list_struct {
-- 
2.15.1

[U-Boot] [PATCH v2 08/11] efi_loader: do_bootefi_exec should always return an EFI status code

[Heinrich Schuchardt]

The return type of do_bootefi_exec() is efi_status_t. So in case
of an error we should always return an EFI status code.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	no change, patch resent
---
 cmd/bootefi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index dff86cf9f9a..7d4100ceeb9 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -164,7 +164,7 @@ static efi_status_t do_bootefi_exec(void *efi, void *fdt,
 	struct efi_loaded_image loaded_image_info = {};
 	struct efi_object loaded_image_info_obj = {};
 	struct efi_device_path *memdp = NULL;
-	ulong ret;
+	efi_status_t ret;
 
 	EFIAPI efi_status_t (*entry)(efi_handle_t image_handle,
 				     struct efi_system_table *st);
@@ -229,7 +229,7 @@ static efi_status_t do_bootefi_exec(void *efi, void *fdt,
 	/* Load the EFI payload */
 	entry = efi_load_pe(efi, &loaded_image_info);
 	if (!entry) {
-		ret = -ENOENT;
+		ret = EFI_LOAD_ERROR;
 		goto exit;
 	}
 
-- 
2.15.1

[U-Boot] [PATCH v2 09/11] efi_loader: check initialization of EFI subsystem is successful

[Heinrich Schuchardt]

Up to now errors in the initialization of the EFI subsystems was not
checked.

If any initialization fails, leave the bootefi command.

We do not retry initialization because this would require to undo all prior
initalization steps.

Suggested-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	no change, patch resent
---
 cmd/bootefi.c | 67 +++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 47 insertions(+), 20 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 7d4100ceeb9..3df1d3fbd07 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -22,40 +22,65 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-static u8 efi_obj_list_initialized;
+#define OBJ_LIST_NOT_INITIALIZED 1
+
+static efi_status_t efi_obj_list_initialized = OBJ_LIST_NOT_INITIALIZED;
 
 static struct efi_device_path *bootefi_image_path;
 static struct efi_device_path *bootefi_device_path;
 
 /* Initialize and populate EFI object list */
-static void efi_init_obj_list(void)
+efi_status_t efi_init_obj_list(void)
 {
+	efi_status_t ret = EFI_SUCCESS;
+
 	/* Initialize once only */
-	if (efi_obj_list_initialized)
-		return;
-	efi_obj_list_initialized = 1;
+	if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED)
+		return efi_obj_list_initialized;
 
 	/* Initialize EFI driver uclass */
-	efi_driver_init();
+	ret = efi_driver_init();
+	if (ret != EFI_SUCCESS)
+		goto out;
 
-	efi_console_register();
+	ret = efi_console_register();
+	if (ret != EFI_SUCCESS)
+		goto out;
 #ifdef CONFIG_PARTITIONS
-	efi_disk_register();
+	ret = efi_disk_register();
+	if (ret != EFI_SUCCESS)
+		goto out;
 #endif
 #if defined(CONFIG_LCD) || defined(CONFIG_DM_VIDEO)
-	efi_gop_register();
+	ret = efi_gop_register();
+	if (ret != EFI_SUCCESS)
+		goto out;
 #endif
 #ifdef CONFIG_NET
-	efi_net_register();
+	ret = efi_net_register();
+	if (ret != EFI_SUCCESS)
+		goto out;
 #endif
 #ifdef CONFIG_GENERATE_SMBIOS_TABLE
-	efi_smbios_register();
+	ret = efi_smbios_register();
+	if (ret != EFI_SUCCESS)
+		goto out;
 #endif
-	efi_watchdog_register();
+	ret = efi_watchdog_register();
+	if (ret != EFI_SUCCESS)
+		goto out;
 
 	/* Initialize EFI runtime services */
-	efi_reset_system_init();
-	efi_get_time_init();
+	ret = efi_reset_system_init();
+	if (ret != EFI_SUCCESS)
+		goto out;
+	ret = efi_get_time_init();
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+out:
+	efi_obj_list_initialized = ret;
+	return ret;
 }
 
 /*
@@ -186,9 +211,6 @@ static efi_status_t do_bootefi_exec(void *efi, void *fdt,
 		assert(device_path && image_path);
 	}
 
-	/* Initialize and populate EFI object list */
-	efi_init_obj_list();
-
 	efi_setup_loaded_image(&loaded_image_info, &loaded_image_info_obj,
 			       device_path, image_path);
 
@@ -285,9 +307,6 @@ static int do_bootefi_bootmgr_exec(unsigned long fdt_addr)
 	void *addr;
 	efi_status_t r;
 
-	/* Initialize and populate EFI object list */
-	efi_init_obj_list();
-
 	/*
 	 * gd lives in a fixed register which may get clobbered while we execute
 	 * the payload. So save it here and restore it on every callback entry
@@ -316,6 +335,14 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	unsigned long addr, fdt_addr = 0;
 	efi_status_t r;
 
+	/* Initialize EFI drivers */
+	r = efi_init_obj_list();
+	if (r != EFI_SUCCESS) {
+		printf("Error: Cannot set up EFI drivers, r = %lu\n",
+		       r & ~EFI_ERROR_MASK);
+		return CMD_RET_FAILURE;
+	}
+
 	if (argc < 2)
 		return CMD_RET_USAGE;
 #ifdef CONFIG_CMD_BOOTEFI_HELLO
-- 
2.15.1

[U-Boot] [PATCH v2 10/11] efi_loader: support device tree for bootefi selftest

[Heinrich Schuchardt]

The second argument of the bootefi command should always be usable to
specify a device tree. This was missing for bootefi selftest and
bootefi hello.

Proper error handling is added.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	new patch
---
 cmd/bootefi.c                 | 109 ++++++++++++++++++++++++------------------
 include/efi_loader.h          |   2 +
 lib/efi_loader/efi_boottime.c |   2 +
 3 files changed, 67 insertions(+), 46 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 3df1d3fbd07..357543b7750 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -178,11 +178,49 @@ static efi_status_t efi_run_in_el2(EFIAPI efi_status_t (*entry)(
 }
 #endif
 
+static efi_status_t efi_install_fdt(void *fdt)
+{
+	bootm_headers_t img = { 0 };
+	ulong fdt_pages, fdt_size, fdt_start, fdt_end;
+	efi_status_t ret;
+
+	if (fdt_check_header(fdt)) {
+		printf("ERROR: invalid device tree\n");
+		return EFI_INVALID_PARAMETER;
+	}
+
+	/* Prepare fdt for payload */
+	fdt = copy_fdt(fdt);
+	if (!fdt)
+		return EFI_OUT_OF_RESOURCES;
+
+	if (image_setup_libfdt(&img, fdt, 0, NULL)) {
+		printf("ERROR: failed to process device tree\n");
+		return EFI_LOAD_ERROR;
+	}
+
+	/* Link to it in the efi tables */
+	ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
+	if (ret != EFI_SUCCESS)
+		return EFI_OUT_OF_RESOURCES;
+
+	/* And reserve the space in the memory map */
+	fdt_start = ((ulong)fdt) & ~EFI_PAGE_MASK;
+	fdt_end = ((ulong)fdt) + fdt_totalsize(fdt);
+	fdt_size = (fdt_end - fdt_start) + EFI_PAGE_MASK;
+	fdt_pages = fdt_size >> EFI_PAGE_SHIFT;
+	/* Give a bootloader the chance to modify the device tree */
+	fdt_pages += 2;
+	ret = efi_add_memory_map(fdt_start, fdt_pages,
+				 EFI_BOOT_SERVICES_DATA, true);
+	return ret;
+}
+
 /*
  * Load an EFI payload into a newly allocated piece of memory, register all
  * EFI objects it would want to access and jump to it.
  */
-static efi_status_t do_bootefi_exec(void *efi, void *fdt,
+static efi_status_t do_bootefi_exec(void *efi,
 				    struct efi_device_path *device_path,
 				    struct efi_device_path *image_path)
 {
@@ -193,9 +231,6 @@ static efi_status_t do_bootefi_exec(void *efi, void *fdt,
 
 	EFIAPI efi_status_t (*entry)(efi_handle_t image_handle,
 				     struct efi_system_table *st);
-	ulong fdt_pages, fdt_size, fdt_start, fdt_end;
-	const efi_guid_t fdt_guid = EFI_FDT_GUID;
-	bootm_headers_t img = { 0 };
 
 	/*
 	 * Special case for efi payload not loaded from disk, such as
@@ -220,32 +255,6 @@ static efi_status_t do_bootefi_exec(void *efi, void *fdt,
 	 */
 	efi_save_gd();
 
-	if (fdt && !fdt_check_header(fdt)) {
-		/* Prepare fdt for payload */
-		fdt = copy_fdt(fdt);
-
-		if (image_setup_libfdt(&img, fdt, 0, NULL)) {
-			printf("ERROR: Failed to process device tree\n");
-			return -EINVAL;
-		}
-
-		/* Link to it in the efi tables */
-		efi_install_configuration_table(&fdt_guid, fdt);
-
-		/* And reserve the space in the memory map */
-		fdt_start = ((ulong)fdt) & ~EFI_PAGE_MASK;
-		fdt_end = ((ulong)fdt) + fdt_totalsize(fdt);
-		fdt_size = (fdt_end - fdt_start) + EFI_PAGE_MASK;
-		fdt_pages = fdt_size >> EFI_PAGE_SHIFT;
-		/* Give a bootloader the chance to modify the device tree */
-		fdt_pages += 2;
-		efi_add_memory_map(fdt_start, fdt_pages,
-				   EFI_BOOT_SERVICES_DATA, true);
-	} else {
-		printf("WARNING: Invalid device tree, expect boot to fail\n");
-		efi_install_configuration_table(&fdt_guid, NULL);
-	}
-
 	/* Transfer environment variable bootargs as load options */
 	set_load_options(&loaded_image_info, "bootargs");
 	/* Load the EFI payload */
@@ -301,7 +310,7 @@ exit:
 	return ret;
 }
 
-static int do_bootefi_bootmgr_exec(unsigned long fdt_addr)
+static int do_bootefi_bootmgr_exec(void)
 {
 	struct efi_device_path *device_path, *file_path;
 	void *addr;
@@ -318,7 +327,7 @@ static int do_bootefi_bootmgr_exec(unsigned long fdt_addr)
 		return 1;
 
 	printf("## Starting EFI application at %p ...\n", addr);
-	r = do_bootefi_exec(addr, (void *)fdt_addr, device_path, file_path);
+	r = do_bootefi_exec(addr, device_path, file_path);
 	printf("## Application terminated, r = %lu\n",
 	       r & ~EFI_ERROR_MASK);
 
@@ -331,9 +340,10 @@ static int do_bootefi_bootmgr_exec(unsigned long fdt_addr)
 /* Interpreter command to boot an arbitrary EFI image from memory */
 static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
-	char *saddr, *sfdt;
-	unsigned long addr, fdt_addr = 0;
+	unsigned long addr;
+	char *saddr;
 	efi_status_t r;
+	void *fdt_addr;
 
 	/* Initialize EFI drivers */
 	r = efi_init_obj_list();
@@ -345,6 +355,22 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 	if (argc < 2)
 		return CMD_RET_USAGE;
+
+	if (argc > 2) {
+		fdt_addr = (void *)simple_strtoul(argv[2], NULL, 16);
+		if (!fdt_addr && *argv[2] != '0')
+			return CMD_RET_USAGE;
+		/* Install device tree */
+		r = efi_install_fdt(fdt_addr);
+		if (r != EFI_SUCCESS) {
+			printf("ERROR: failed to install device tree\n");
+			return CMD_RET_FAILURE;
+		}
+	} else {
+		/* Remove device tree. EFI_NOT_FOUND can be ignored here */
+		efi_install_configuration_table(&efi_guid_fdt, NULL);
+		printf("WARNING: booting without device tree\n");
+	}
 #ifdef CONFIG_CMD_BOOTEFI_HELLO
 	if (!strcmp(argv[1], "hello")) {
 		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
@@ -390,12 +416,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	} else
 #endif
 	if (!strcmp(argv[1], "bootmgr")) {
-		unsigned long fdt_addr = 0;
-
-		if (argc > 2)
-			fdt_addr = simple_strtoul(argv[2], NULL, 16);
-
-		return do_bootefi_bootmgr_exec(fdt_addr);
+		return do_bootefi_bootmgr_exec();
 	} else {
 		saddr = argv[1];
 
@@ -404,15 +425,11 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		if (!addr && *saddr != '0')
 			return CMD_RET_USAGE;
 
-		if (argc > 2) {
-			sfdt = argv[2];
-			fdt_addr = simple_strtoul(sfdt, NULL, 16);
-		}
 	}
 
 	printf("## Starting EFI application at %08lx ...\n", addr);
-	r = do_bootefi_exec((void *)addr, (void *)fdt_addr,
-			    bootefi_device_path, bootefi_image_path);
+	r = do_bootefi_exec((void *)addr, bootefi_device_path,
+			    bootefi_image_path);
 	printf("## Application terminated, r = %lu\n",
 	       r & ~EFI_ERROR_MASK);
 
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 85a47ca2f5e..3ccbb98aaf9 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -93,6 +93,8 @@ extern const efi_guid_t efi_guid_console_control;
 extern const efi_guid_t efi_guid_device_path;
 /* GUID of the EFI_DRIVER_BINDING_PROTOCOL */
 extern const efi_guid_t efi_guid_driver_binding_protocol;
+/* GUID of the device tree table */
+extern const efi_guid_t efi_guid_fdt;
 extern const efi_guid_t efi_guid_loaded_image;
 extern const efi_guid_t efi_guid_device_path_to_text_protocol;
 extern const efi_guid_t efi_simple_file_system_protocol_guid;
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 6eea2395c7b..64d82386753 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -56,6 +56,8 @@ static volatile void *efi_gd, *app_gd;
 
 static int entry_count;
 static int nesting_level;
+/* GUID of the device tree table */
+const efi_guid_t efi_guid_fdt = EFI_FDT_GUID;
 /* GUID of the EFI_DRIVER_BINDING_PROTOCOL */
 const efi_guid_t efi_guid_driver_binding_protocol =
 			EFI_DRIVER_BINDING_PROTOCOL_GUID;
-- 
2.15.1

[U-Boot] [PATCH v2 11/11] efi_selftest: check installation of the device tree

[Heinrich Schuchardt]

The unit test checks if a device tree is installed. It requires that the
'compatible' property of the root node exists. If available it prints the
'serial-number' property.

The serial-number property is derived from the environment variable
'serial#'. This can be used to check if the image_setup_libfdt() function
is executed.

A Python test is supplied. It sets a value for serial# and checks that the
selftest shows this as serial-number.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	new patch
---
 lib/efi_selftest/Makefile           |   1 +
 lib/efi_selftest/efi_selftest_fdt.c | 188 ++++++++++++++++++++++++++++++++++++
 test/py/tests/test_efi_selftest.py  |  14 +++
 3 files changed, 203 insertions(+)
 create mode 100644 lib/efi_selftest/efi_selftest_fdt.c

diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
index c4bdbdf6c05..88c44840d52 100644
--- a/lib/efi_selftest/Makefile
+++ b/lib/efi_selftest/Makefile
@@ -19,6 +19,7 @@ efi_selftest_console.o \
 efi_selftest_devicepath.o \
 efi_selftest_events.o \
 efi_selftest_exitbootservices.o \
+efi_selftest_fdt.o \
 efi_selftest_gop.o \
 efi_selftest_manageprotocols.o \
 efi_selftest_snp.o \
diff --git a/lib/efi_selftest/efi_selftest_fdt.c b/lib/efi_selftest/efi_selftest_fdt.c
new file mode 100644
index 00000000000..24db0dcf7d5
--- /dev/null
+++ b/lib/efi_selftest/efi_selftest_fdt.c
@@ -0,0 +1,188 @@
+/*
+ * efi_selftest_pos
+ *
+ * Copyright (c) 2018 Heinrich Schuchardt <xypron.glpk@gmx.de>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ *
+ * Test the EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.
+ *
+ * The following services are tested:
+ * OutputString, TestString, SetAttribute.
+ */
+
+#include <efi_selftest.h>
+#include <libfdt.h>
+
+static struct efi_boot_services *boottime;
+static const char *fdt;
+
+/* This should be sufficent for */
+#define BUFFERSIZE 0x100000
+
+static efi_guid_t fdt_guid = EFI_FDT_GUID;
+
+/*
+ * Convert FDT value to host endianness.
+ *
+ * @val		FDT value
+ * @return	converted value
+ */
+static uint32_t f2h(fdt32_t val)
+{
+	char *buf = (char *)&val;
+	char i;
+
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+	/* Swap the bytes */
+	i = buf[0]; buf[0] = buf[3]; buf[3] = i;
+	i = buf[1]; buf[1] = buf[2]; buf[2] = i;
+#endif
+	return *(uint32_t *)buf;
+}
+
+/*
+ * Return the value of a property of the FDT root node.
+ *
+ * @name	name of the property
+ * @return	value of the property
+ */
+static char *get_property(const u16 *property)
+{
+	struct fdt_header *header = (struct fdt_header *)fdt;
+	const fdt32_t *pos;
+	const char *strings;
+
+	if (!header)
+		return NULL;
+
+	if (f2h(header->magic) != FDT_MAGIC) {
+		printf("Wrong magic\n");
+		return NULL;
+	}
+
+	pos = (fdt32_t *)(fdt + f2h(header->off_dt_struct));
+	strings = fdt + f2h(header->off_dt_strings);
+
+	for (;;) {
+		switch (f2h(pos[0])) {
+		case FDT_BEGIN_NODE: {
+			char *c = (char *)&pos[1];
+			size_t i;
+
+			for (i = 0; c[i]; ++i)
+				;
+			pos = &pos[2 + (i >> 2)];
+			break;
+		}
+		case FDT_PROP: {
+			struct fdt_property *prop = (struct fdt_property *)pos;
+			const char *label = &strings[f2h(prop->nameoff)];
+			efi_status_t ret;
+
+			/* Check if this is the property to be returned */
+			if (!efi_st_strcmp_16_8(property, label)) {
+				char *str;
+				efi_uintn_t len = f2h(prop->len);
+
+				if (!len)
+					return NULL;
+				/*
+				 * The string might not be 0 terminated.
+				 * It is safer to make a copy.
+				 */
+				ret = boottime->allocate_pool(
+					EFI_LOADER_DATA, len + 1,
+					(void **)&str);
+				if (ret != EFI_SUCCESS) {
+					efi_st_printf("AllocatePool failed\n");
+					return NULL;
+				}
+				boottime->copy_mem(str, &pos[3], len);
+				str[len] = 0;
+
+				return str;
+			}
+
+			pos = &pos[3 + ((f2h(prop->len) + 3) >> 2)];
+			break;
+		}
+		case FDT_NOP:
+			pos = &pos[1];
+			break;
+		default:
+			return NULL;
+		}
+	}
+}
+
+/*
+ * Setup unit test.
+ *
+ * @handle:	handle of the loaded image
+ * @systable:	system table
+ * @return:	EFI_ST_SUCCESS for success
+ */
+static int setup(const efi_handle_t img_handle,
+		 const struct efi_system_table *systable)
+{
+	efi_uintn_t i;
+
+	boottime = systable->boottime;
+
+	/* Find configuration tables */
+	for (i = 0; i < systable->nr_tables; ++i) {
+		if (!efi_st_memcmp(&systable->tables[i].guid, &fdt_guid,
+				   sizeof(efi_guid_t)))
+			fdt = systable->tables[i].table;
+	}
+	if (!fdt) {
+		efi_st_error("Missing device tree\n");
+		return EFI_ST_FAILURE;
+	}
+
+	return EFI_ST_SUCCESS;
+}
+
+/*
+ * Execute unit test.
+ *
+ * @return:	EFI_ST_SUCCESS for success
+ */
+static int execute(void)
+{
+	char *str;
+	efi_status_t ret;
+
+	str = get_property(L"compatible");
+	if (str) {
+		efi_st_printf("compatible: %s\n", str);
+		ret = boottime->free_pool(str);
+		if (ret != EFI_SUCCESS) {
+			efi_st_error("FreePool failed\n");
+			return EFI_ST_FAILURE;
+		}
+	} else {
+		efi_st_printf("Missing property 'compatible'\n");
+		return EFI_ST_FAILURE;
+	}
+	str = get_property(L"serial-number");
+	if (str) {
+		efi_st_printf("serial-number: %s\n", str);
+		ret = boottime->free_pool(str);
+		if (ret != EFI_SUCCESS) {
+			efi_st_error("FreePool failed\n");
+			return EFI_ST_FAILURE;
+		}
+	}
+
+	return EFI_ST_SUCCESS;
+}
+
+EFI_UNIT_TEST(fdt) = {
+	.name = "device tree",
+	.phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
+	.setup = setup,
+	.execute = execute,
+	.on_request = true,
+};
diff --git a/test/py/tests/test_efi_selftest.py b/test/py/tests/test_efi_selftest.py
index 66b799bed66..b1ef6bd5811 100644
--- a/test/py/tests/test_efi_selftest.py
+++ b/test/py/tests/test_efi_selftest.py
@@ -24,6 +24,20 @@ def test_efi_selftest(u_boot_console):
 		raise Exception('Reset failed during the EFI selftest')
 	u_boot_console.restart_uboot();
 
+ at pytest.mark.buildconfigspec('cmd_bootefi_selftest')
+ at pytest.mark.buildconfigspec('of_control')
+def test_efi_selftest_device_tree(u_boot_console):
+	u_boot_console.run_command(cmd='setenv efi_selftest list')
+	output = u_boot_console.run_command('bootefi selftest')
+	assert '\'device tree\'' in output
+	u_boot_console.run_command(cmd='setenv efi_selftest device tree')
+	u_boot_console.run_command(cmd='setenv -f serial# Testing DT')
+	u_boot_console.run_command(cmd='bootefi selftest ${fdtcontroladdr}', wait_for_prompt=False)
+	m = u_boot_console.p.expect(['serial-number: Testing DT', 'U-Boot'])
+	if m != 0:
+		raise Exception('Reset failed in \'device tree\' test')
+	u_boot_console.restart_uboot();
+
 @pytest.mark.buildconfigspec('cmd_bootefi_selftest')
 def test_efi_selftest_watchdog_reboot(u_boot_console):
 	u_boot_console.run_command(cmd='setenv efi_selftest list')
-- 
2.15.1

[U-Boot] [PATCH v2 01/11] efi_loader: efi_smbios_register should have a return value

[Simon Glass]

On 15 February 2018 at 00:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Errors may occur inside efi_smbios_register().
>
> - Return a status code.
> - Remove unused variables.
> - Use constants where applicable.
>
> Suggested-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
>         remove a change in unrelated code
>         resent with this patch series
> ---
>  include/efi_loader.h        |  2 +-
>  lib/efi_loader/efi_smbios.c | 23 +++++++++++++++--------
>  2 files changed, 16 insertions(+), 9 deletions(-)

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

[U-Boot] [PATCH v2 02/11] efi_loader: return efi_status_t from efi_gop_register

[Simon Glass]

Hi Heinrich,

On 15 February 2018 at 00:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> All initialization routines should return a status code instead of
> a boolean.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
>         new patch
> ---
>  include/efi_loader.h     |  2 +-
>  lib/efi_loader/efi_gop.c | 34 ++++++++++++++++++++++------------
>  2 files changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 72c83fd5033..779b8bde2e3 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -179,7 +179,7 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
>                                const char *if_typename, int diskid,
>                                const char *pdevname);
>  /* Called by bootefi to make GOP (graphical) interface available */
> -int efi_gop_register(void);
> +efi_status_t efi_gop_register(void);
>  /* Called by bootefi to make the network interface available */
>  int efi_net_register(void);
>  /* Called by bootefi to make the watchdog available */
> diff --git a/lib/efi_loader/efi_gop.c b/lib/efi_loader/efi_gop.c
> index 3caddd5f844..91b0b6a064b 100644
> --- a/lib/efi_loader/efi_gop.c
> +++ b/lib/efi_loader/efi_gop.c
> @@ -125,8 +125,13 @@ efi_status_t EFIAPI gop_blt(struct efi_gop *this, void *buffer,
>         return EFI_EXIT(EFI_SUCCESS);
>  }
>
> -/* This gets called from do_bootefi_exec(). */
> -int efi_gop_register(void)
> +/*
> + * Install graphical output protocol.
> + *
> + * If no supported video device exists this is not considered as an
> + * error.
> + */

This comment should be in the header file so people can see the API in
one place.

It's unfortunate that U-Boot error codes get lost here. Perhaps it
does not make sense to return them and have the caller report the
error? I'm not sure what is best, but one symptom of the current
approach is the use of printf() to report (and suppress) the error.

Regards,
Simon

[U-Boot] [PATCH v2 03/11] efi_loader: return efi_status_t from efi_net_register

[Simon Glass]

Hi,

On 15 February 2018 at 00:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Consistently return status codes form efi_net_register().
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
>         new patch
> ---
>  include/efi_loader.h     |  2 +-
>  lib/efi_loader/efi_net.c | 24 +++++++++++++-----------
>  2 files changed, 14 insertions(+), 12 deletions(-)
>

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

I have a similar comment to the previous patch re using printf() to
report errors. Ideally the error should be returned to the caller (and
perhaps logged with log() or debug()).

Regards,
Simon

[U-Boot] [PATCH v2 04/11] efi_loader: consistently return efi_status_t efi_watchdog_register

[Simon Glass]

On 15 February 2018 at 00:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> efi_watchdog_register() should always return a status code and not
> a boolean value.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
>         new patch
> ---
>  include/efi_loader.h          | 2 +-
>  lib/efi_loader/efi_watchdog.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)

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

[U-Boot] [PATCH v2 05/11] efi_loader: simplify calling efi_init_obj_list

[Simon Glass]

On 15 February 2018 at 00:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> efi_init_obj_list() should be executed only once.
>
> Rather than having the caller check this variable and the callee set it,
> move all access to the variable inside the function. This reduces the
> logic needed to call efi_init_obj_list().
>
> Suggested-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
>         no change, patch resent
> ---
>  cmd/bootefi.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

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

[U-Boot] [PATCH v2 06/11] efi_loader: exit status for efi_reset_system_init

[Simon Glass]

On 15 February 2018 at 00:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> efi_reset_system_init provides the architecture or board specific
> initialization of the EFI subsystem. Errors should be caught and
> signalled by a return code.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
>         new patch
> ---
>  arch/arm/cpu/armv8/fsl-layerscape/cpu.c |  4 ++--
>  arch/arm/mach-bcm283x/reset.c           |  4 ++--
>  include/efi_loader.h                    | 11 ++++++++---
>  lib/efi_loader/efi_runtime.c            | 15 ++++++++++++---
>  4 files changed, 24 insertions(+), 10 deletions(-)

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

Suggestion below

>
> -void efi_add_runtime_mmio(void *mmio_ptr, u64 len)
> +efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len)
>  {
>         struct efi_runtime_mmio_list *newmmio;
> +       efi_status_t ret;
>
>         u64 pages = (len + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
> -       efi_add_memory_map(*(uintptr_t *)mmio_ptr, pages, EFI_MMAP_IO, false);
> +       ret = efi_add_memory_map(*(uintptr_t *)mmio_ptr, pages, EFI_MMAP_IO,
> +                                false);
> +       if (ret != EFI_SUCCESS)

debug() or log() here?

> +               return ret;
>
>         newmmio = calloc(1, sizeof(*newmmio));
> +       if (!newmmio)
> +               return EFI_OUT_OF_RESOURCES;
>         newmmio->ptr = mmio_ptr;
>         newmmio->paddr = *(uintptr_t *)mmio_ptr;
>         newmmio->len = len;
>         list_add_tail(&newmmio->link, &efi_runtime_mmio);
> +
> +       return ret;
>  }
>
>  /*
> --
> 2.15.1
>

[U-Boot] [PATCH v2 07/11] efi_loader: efi_get_time_init should return status code

[Simon Glass]

On 15 February 2018 at 00:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> All EFI initialization functions should return a status code.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
>         new patch
> ---
>  include/efi_loader.h         | 2 +-
>  lib/efi_loader/efi_runtime.c | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)

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

[U-Boot] [PATCH v2 08/11] efi_loader: do_bootefi_exec should always return an EFI status code

[Simon Glass]

On 15 February 2018 at 00:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> The return type of do_bootefi_exec() is efi_status_t. So in case
> of an error we should always return an EFI status code.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
>         no change, patch resent
> ---
>  cmd/bootefi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

[U-Boot] [PATCH v2 09/11] efi_loader: check initialization of EFI subsystem is successful

[Simon Glass]

On 15 February 2018 at 00:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Up to now errors in the initialization of the EFI subsystems was not
> checked.
>
> If any initialization fails, leave the bootefi command.
>
> We do not retry initialization because this would require to undo all prior
> initalization steps.

Also, why would it succeed on the second try?
>
> Suggested-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
>         no change, patch resent
> ---
>  cmd/bootefi.c | 67 +++++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 47 insertions(+), 20 deletions(-)

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

[U-Boot] [PATCH v2 10/11] efi_loader: support device tree for bootefi selftest

[Simon Glass]

On 15 February 2018 at 00:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> The second argument of the bootefi command should always be usable to
> specify a device tree. This was missing for bootefi selftest and
> bootefi hello.
>
> Proper error handling is added.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
>         new patch
> ---
>  cmd/bootefi.c                 | 109 ++++++++++++++++++++++++------------------
>  include/efi_loader.h          |   2 +
>  lib/efi_loader/efi_boottime.c |   2 +
>  3 files changed, 67 insertions(+), 46 deletions(-)

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

[U-Boot] [PATCH v2 11/11] efi_selftest: check installation of the device tree

[Simon Glass]

Hi Heinrich,

On 15 February 2018 at 00:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> The unit test checks if a device tree is installed. It requires that the
> 'compatible' property of the root node exists. If available it prints the
> 'serial-number' property.
>
> The serial-number property is derived from the environment variable
> 'serial#'. This can be used to check if the image_setup_libfdt() function
> is executed.
>
> A Python test is supplied. It sets a value for serial# and checks that the
> selftest shows this as serial-number.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
>         new patch
> ---
>  lib/efi_selftest/Makefile           |   1 +
>  lib/efi_selftest/efi_selftest_fdt.c | 188 ++++++++++++++++++++++++++++++++++++
>  test/py/tests/test_efi_selftest.py  |  14 +++
>  3 files changed, 203 insertions(+)
>  create mode 100644 lib/efi_selftest/efi_selftest_fdt.c
>
> diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
> index c4bdbdf6c05..88c44840d52 100644
> --- a/lib/efi_selftest/Makefile
> +++ b/lib/efi_selftest/Makefile
> @@ -19,6 +19,7 @@ efi_selftest_console.o \
>  efi_selftest_devicepath.o \
>  efi_selftest_events.o \
>  efi_selftest_exitbootservices.o \
> +efi_selftest_fdt.o \
>  efi_selftest_gop.o \
>  efi_selftest_manageprotocols.o \
>  efi_selftest_snp.o \
> diff --git a/lib/efi_selftest/efi_selftest_fdt.c b/lib/efi_selftest/efi_selftest_fdt.c
> new file mode 100644
> index 00000000000..24db0dcf7d5
> --- /dev/null
> +++ b/lib/efi_selftest/efi_selftest_fdt.c
> @@ -0,0 +1,188 @@
> +/*
> + * efi_selftest_pos
> + *
> + * Copyright (c) 2018 Heinrich Schuchardt <xypron.glpk@gmx.de>
> + *
> + * SPDX-License-Identifier:     GPL-2.0+
> + *
> + * Test the EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.
> + *
> + * The following services are tested:
> + * OutputString, TestString, SetAttribute.
> + */
> +
> +#include <efi_selftest.h>
> +#include <libfdt.h>
> +
> +static struct efi_boot_services *boottime;
> +static const char *fdt;
> +
> +/* This should be sufficent for */
> +#define BUFFERSIZE 0x100000
> +
> +static efi_guid_t fdt_guid = EFI_FDT_GUID;
> +
> +/*
> + * Convert FDT value to host endianness.
> + *
> + * @val                FDT value
> + * @return     converted value
> + */
> +static uint32_t f2h(fdt32_t val)

fdt32_to_cpu() ?

> +{
> +       char *buf = (char *)&val;
> +       char i;
> +
> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> +       /* Swap the bytes */
> +       i = buf[0]; buf[0] = buf[3]; buf[3] = i;
> +       i = buf[1]; buf[1] = buf[2]; buf[2] = i;
> +#endif
> +       return *(uint32_t *)buf;
> +}
> +
> +/*
> + * Return the value of a property of the FDT root node.
> + *
> + * @name       name of the property
> + * @return     value of the property
> + */
> +static char *get_property(const u16 *property)

We should use libfdt here as it already has this implementation.
[..]

> +/*
> + * Setup unit test.
> + *
> + * @handle:    handle of the loaded image
> + * @systable:  system table
> + * @return:    EFI_ST_SUCCESS for success
> + */
> +static int setup(const efi_handle_t img_handle,
> +                const struct efi_system_table *systable)
> +{
> +       efi_uintn_t i;
> +
> +       boottime = systable->boottime;
> +
> +       /* Find configuration tables */
> +       for (i = 0; i < systable->nr_tables; ++i) {
> +               if (!efi_st_memcmp(&systable->tables[i].guid, &fdt_guid,
> +                                  sizeof(efi_guid_t)))
> +                       fdt = systable->tables[i].table;
> +       }
> +       if (!fdt) {
> +               efi_st_error("Missing device tree\n");
> +               return EFI_ST_FAILURE;
> +       }
> +
> +       return EFI_ST_SUCCESS;
> +}
> +
> +/*
> + * Execute unit test.
> + *
> + * @return:    EFI_ST_SUCCESS for success
> + */
> +static int execute(void)
> +{
> +       char *str;
> +       efi_status_t ret;
> +
> +       str = get_property(L"compatible");

Why do we use unicode for the property name?

> +       if (str) {
> +               efi_st_printf("compatible: %s\n", str);
> +               ret = boottime->free_pool(str);
> +               if (ret != EFI_SUCCESS) {
> +                       efi_st_error("FreePool failed\n");
> +                       return EFI_ST_FAILURE;
> +               }
> +       } else {
> +               efi_st_printf("Missing property 'compatible'\n");
> +               return EFI_ST_FAILURE;
> +       }
> +       str = get_property(L"serial-number");
> +       if (str) {
> +               efi_st_printf("serial-number: %s\n", str);
> +               ret = boottime->free_pool(str);
> +               if (ret != EFI_SUCCESS) {
> +                       efi_st_error("FreePool failed\n");
> +                       return EFI_ST_FAILURE;
> +               }
> +       }
> +
> +       return EFI_ST_SUCCESS;
> +}
> +
> +EFI_UNIT_TEST(fdt) = {
> +       .name = "device tree",
> +       .phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
> +       .setup = setup,
> +       .execute = execute,
> +       .on_request = true,
> +};
> diff --git a/test/py/tests/test_efi_selftest.py b/test/py/tests/test_efi_selftest.py
> index 66b799bed66..b1ef6bd5811 100644
> --- a/test/py/tests/test_efi_selftest.py
> +++ b/test/py/tests/test_efi_selftest.py
> @@ -24,6 +24,20 @@ def test_efi_selftest(u_boot_console):
>                 raise Exception('Reset failed during the EFI selftest')
>         u_boot_console.restart_uboot();
>
> + at pytest.mark.buildconfigspec('cmd_bootefi_selftest')
> + at pytest.mark.buildconfigspec('of_control')
> +def test_efi_selftest_device_tree(u_boot_console):
> +       u_boot_console.run_command(cmd='setenv efi_selftest list')
> +       output = u_boot_console.run_command('bootefi selftest')
> +       assert '\'device tree\'' in output
> +       u_boot_console.run_command(cmd='setenv efi_selftest device tree')
> +       u_boot_console.run_command(cmd='setenv -f serial# Testing DT')
> +       u_boot_console.run_command(cmd='bootefi selftest ${fdtcontroladdr}', wait_for_prompt=False)
> +       m = u_boot_console.p.expect(['serial-number: Testing DT', 'U-Boot'])

Looks like a good test for this feature.

> +       if m != 0:
> +               raise Exception('Reset failed in \'device tree\' test')
> +       u_boot_console.restart_uboot();
> +
>  @pytest.mark.buildconfigspec('cmd_bootefi_selftest')
>  def test_efi_selftest_watchdog_reboot(u_boot_console):
>         u_boot_console.run_command(cmd='setenv efi_selftest list')
> --
> 2.15.1
>

Regards,
Simon

[U-Boot] [PATCH v2 11/11] efi_selftest: check installation of the device tree

[Heinrich Schuchardt]

On 03/23/2018 03:30 PM, Simon Glass wrote:
> Hi Heinrich,
> 
> On 15 February 2018 at 00:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> The unit test checks if a device tree is installed. It requires that the
>> 'compatible' property of the root node exists. If available it prints the
>> 'serial-number' property.
>>
>> The serial-number property is derived from the environment variable
>> 'serial#'. This can be used to check if the image_setup_libfdt() function
>> is executed.
>>
>> A Python test is supplied. It sets a value for serial# and checks that the
>> selftest shows this as serial-number.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> v2
>>         new patch
>> ---
>>  lib/efi_selftest/Makefile           |   1 +
>>  lib/efi_selftest/efi_selftest_fdt.c | 188 ++++++++++++++++++++++++++++++++++++
>>  test/py/tests/test_efi_selftest.py  |  14 +++
>>  3 files changed, 203 insertions(+)
>>  create mode 100644 lib/efi_selftest/efi_selftest_fdt.c
>>
>> diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
>> index c4bdbdf6c05..88c44840d52 100644
>> --- a/lib/efi_selftest/Makefile
>> +++ b/lib/efi_selftest/Makefile
>> @@ -19,6 +19,7 @@ efi_selftest_console.o \
>>  efi_selftest_devicepath.o \
>>  efi_selftest_events.o \
>>  efi_selftest_exitbootservices.o \
>> +efi_selftest_fdt.o \
>>  efi_selftest_gop.o \
>>  efi_selftest_manageprotocols.o \
>>  efi_selftest_snp.o \
>> diff --git a/lib/efi_selftest/efi_selftest_fdt.c b/lib/efi_selftest/efi_selftest_fdt.c
>> new file mode 100644
>> index 00000000000..24db0dcf7d5
>> --- /dev/null
>> +++ b/lib/efi_selftest/efi_selftest_fdt.c
>> @@ -0,0 +1,188 @@
>> +/*
>> + * efi_selftest_pos
>> + *
>> + * Copyright (c) 2018 Heinrich Schuchardt <xypron.glpk@gmx.de>
>> + *
>> + * SPDX-License-Identifier:     GPL-2.0+
>> + *
>> + * Test the EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.
>> + *
>> + * The following services are tested:
>> + * OutputString, TestString, SetAttribute.
>> + */
>> +
>> +#include <efi_selftest.h>
>> +#include <libfdt.h>
>> +
>> +static struct efi_boot_services *boottime;
>> +static const char *fdt;
>> +
>> +/* This should be sufficent for */
>> +#define BUFFERSIZE 0x100000
>> +
>> +static efi_guid_t fdt_guid = EFI_FDT_GUID;
>> +
>> +/*
>> + * Convert FDT value to host endianness.
>> + *
>> + * @val                FDT value
>> + * @return     converted value
>> + */
>> +static uint32_t f2h(fdt32_t val)
> 
> fdt32_to_cpu() ?

Thank you for reviewing.

Up to now I have kept lib/efi_selftest self contained so that we could
compile it as an EFI executable in future (like lib/efi_loader/helloword.c).

If we reject this idea we could simplify different parts of the coding.

@Alex:
What is your idea where we should head?

> 
>> +{
>> +       char *buf = (char *)&val;
>> +       char i;
>> +
>> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>> +       /* Swap the bytes */
>> +       i = buf[0]; buf[0] = buf[3]; buf[3] = i;
>> +       i = buf[1]; buf[1] = buf[2]; buf[2] = i;
>> +#endif
>> +       return *(uint32_t *)buf;
>> +}
>> +
>> +/*
>> + * Return the value of a property of the FDT root node.
>> + *
>> + * @name       name of the property
>> + * @return     value of the property
>> + */
>> +static char *get_property(const u16 *property)
> 
> We should use libfdt here as it already has this implementation.
> [..]
> 
>> +/*
>> + * Setup unit test.
>> + *
>> + * @handle:    handle of the loaded image
>> + * @systable:  system table
>> + * @return:    EFI_ST_SUCCESS for success
>> + */
>> +static int setup(const efi_handle_t img_handle,
>> +                const struct efi_system_table *systable)
>> +{
>> +       efi_uintn_t i;
>> +
>> +       boottime = systable->boottime;
>> +
>> +       /* Find configuration tables */
>> +       for (i = 0; i < systable->nr_tables; ++i) {
>> +               if (!efi_st_memcmp(&systable->tables[i].guid, &fdt_guid,
>> +                                  sizeof(efi_guid_t)))
>> +                       fdt = systable->tables[i].table;
>> +       }
>> +       if (!fdt) {
>> +               efi_st_error("Missing device tree\n");
>> +               return EFI_ST_FAILURE;
>> +       }
>> +
>> +       return EFI_ST_SUCCESS;
>> +}
>> +
>> +/*
>> + * Execute unit test.
>> + *
>> + * @return:    EFI_ST_SUCCESS for success
>> + */
>> +static int execute(void)
>> +{
>> +       char *str;
>> +       efi_status_t ret;
>> +
>> +       str = get_property(L"compatible");
> 
> Why do we use unicode for the property name?

See comment above. efi_st_strcmp_16_8() can only compare u16 strings to
char*.

Best regards

Heinrich

> 
>> +       if (str) {
>> +               efi_st_printf("compatible: %s\n", str);
>> +               ret = boottime->free_pool(str);
>> +               if (ret != EFI_SUCCESS) {
>> +                       efi_st_error("FreePool failed\n");
>> +                       return EFI_ST_FAILURE;
>> +               }
>> +       } else {
>> +               efi_st_printf("Missing property 'compatible'\n");
>> +               return EFI_ST_FAILURE;
>> +       }
>> +       str = get_property(L"serial-number");
>> +       if (str) {
>> +               efi_st_printf("serial-number: %s\n", str);
>> +               ret = boottime->free_pool(str);
>> +               if (ret != EFI_SUCCESS) {
>> +                       efi_st_error("FreePool failed\n");
>> +                       return EFI_ST_FAILURE;
>> +               }
>> +       }
>> +
>> +       return EFI_ST_SUCCESS;
>> +}
>> +
>> +EFI_UNIT_TEST(fdt) = {
>> +       .name = "device tree",
>> +       .phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
>> +       .setup = setup,
>> +       .execute = execute,
>> +       .on_request = true,
>> +};
>> diff --git a/test/py/tests/test_efi_selftest.py b/test/py/tests/test_efi_selftest.py
>> index 66b799bed66..b1ef6bd5811 100644
>> --- a/test/py/tests/test_efi_selftest.py
>> +++ b/test/py/tests/test_efi_selftest.py
>> @@ -24,6 +24,20 @@ def test_efi_selftest(u_boot_console):
>>                 raise Exception('Reset failed during the EFI selftest')
>>         u_boot_console.restart_uboot();
>>
>> + at pytest.mark.buildconfigspec('cmd_bootefi_selftest')
>> + at pytest.mark.buildconfigspec('of_control')
>> +def test_efi_selftest_device_tree(u_boot_console):
>> +       u_boot_console.run_command(cmd='setenv efi_selftest list')
>> +       output = u_boot_console.run_command('bootefi selftest')
>> +       assert '\'device tree\'' in output
>> +       u_boot_console.run_command(cmd='setenv efi_selftest device tree')
>> +       u_boot_console.run_command(cmd='setenv -f serial# Testing DT')
>> +       u_boot_console.run_command(cmd='bootefi selftest ${fdtcontroladdr}', wait_for_prompt=False)
>> +       m = u_boot_console.p.expect(['serial-number: Testing DT', 'U-Boot'])
> 
> Looks like a good test for this feature.
> 
>> +       if m != 0:
>> +               raise Exception('Reset failed in \'device tree\' test')
>> +       u_boot_console.restart_uboot();
>> +
>>  @pytest.mark.buildconfigspec('cmd_bootefi_selftest')
>>  def test_efi_selftest_watchdog_reboot(u_boot_console):
>>         u_boot_console.run_command(cmd='setenv efi_selftest list')
>> --
>> 2.15.1
>>
> 
> Regards,
> Simon
> 

[U-Boot] [PATCH v2 02/11] efi_loader: return efi_status_t from efi_gop_register

[Heinrich Schuchardt]

On 03/23/2018 03:29 PM, Simon Glass wrote:
> Hi Heinrich,
> 
> On 15 February 2018 at 00:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> All initialization routines should return a status code instead of
>> a boolean.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> v2
>>         new patch
>> ---
>>  include/efi_loader.h     |  2 +-
>>  lib/efi_loader/efi_gop.c | 34 ++++++++++++++++++++++------------
>>  2 files changed, 23 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 72c83fd5033..779b8bde2e3 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -179,7 +179,7 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
>>                                const char *if_typename, int diskid,
>>                                const char *pdevname);
>>  /* Called by bootefi to make GOP (graphical) interface available */
>> -int efi_gop_register(void);
>> +efi_status_t efi_gop_register(void);
>>  /* Called by bootefi to make the network interface available */
>>  int efi_net_register(void);
>>  /* Called by bootefi to make the watchdog available */
>> diff --git a/lib/efi_loader/efi_gop.c b/lib/efi_loader/efi_gop.c
>> index 3caddd5f844..91b0b6a064b 100644
>> --- a/lib/efi_loader/efi_gop.c
>> +++ b/lib/efi_loader/efi_gop.c
>> @@ -125,8 +125,13 @@ efi_status_t EFIAPI gop_blt(struct efi_gop *this, void *buffer,
>>         return EFI_EXIT(EFI_SUCCESS);
>>  }
>>
>> -/* This gets called from do_bootefi_exec(). */
>> -int efi_gop_register(void)
>> +/*
>> + * Install graphical output protocol.
>> + *
>> + * If no supported video device exists this is not considered as an
>> + * error.
>> + */
> 
> This comment should be in the header file so people can see the API in
> one place.
> 
> It's unfortunate that U-Boot error codes get lost here. Perhaps it
> does not make sense to return them and have the caller report the
> error? I'm not sure what is best, but one symptom of the current
> approach is the use of printf() to report (and suppress) the error.

I understand why using log() is a better choice than printf(). We
already added a log category for the EFI subsystem without using it yet.

Do I get your idea right:

You would return an error code to the caller. And if the caller thinks
that the GOP protocol is not needed he should output a log message and
pass on.

As Alex has already picked this patch I would prefer to put such a
change into an extra patch.

Regards

Heinrich