[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Heinrich Schuchardt]

This patch sequence contains all patches needed to load
iPXE and use it for downloading and executing images
via https or http or to mount iSCSI volumes.

Network speed on an Odroid C2 reached 30 MB/s which should be
enough for most use cases.

I have tested the following iPXE commands successfully
* dhcp
* route
* ntp
* sanhook iSCSI-target
* chain http-target
* kernel http-target
* boot (after calling kernel)
* exit
* reboot

The only adjustment in iPXE was adding file src/config/local/nap.h with
  #undef NAP_EFIX86
  #undef NAP_EFIARM
  #define NAP_NULL
and src/config/local/myscript.ipxe with
  #!ipxe
  shell
before building iPXE with
  make bin-arm64-efi/snp.efi EMBED=config/local/myscript.ipxe

The next task will be to put iXPE binaries on a server
and to create Travis CI test cases.

Heinrich Schuchardt (23):
  efi_loader: allow return value in EFI_CALL
  efi_loader: notify when ExitBootServices is invoked
  efi_loader: support 16 protocols per efi_object
  efi_loader: rework efi_locate_handle
  efi_loader: rework efi_search_obj
  efi_loader: new function efi_search_protocol
  efi_loader: simplify efi_install_protocol_interface
  efi_loader: allow creating new handles
  efi_loader: simplify efi_uninstall_protocol_interface
  efi_loader: open_info in OpenProtocol
  efi_loader: open_info in CloseProtocol
  efi_loader: implement OpenProtocolInformation
  efi_loader: non-static efi_open_protocol, efi_close_protocol
  efi_loader: pass GUIDs as const efi_guid_t *
  efi_loader: implement ConnectController
  efi_loader: implement DisconnectController
  efi_loader: efi_net: hwaddr_size = 6
  efi_net: return EFI_UNSUPPORTED where appropriate
  efi_loader: correct bits of receive_filters bit mask
  efi_loader: use events for efi_net_receive
  efi_loader: fix efi_net_get_status
  efi_loader: set parent handle in efi_load_image
  efi_loader: implement SetWatchdogTimer

 cmd/bootefi.c                 |   1 +
 include/efi_api.h             |  83 +++--
 include/efi_loader.h          |  38 ++-
 lib/efi_loader/Makefile       |   2 +-
 lib/efi_loader/efi_boottime.c | 729 ++++++++++++++++++++++++++++++++----------
 lib/efi_loader/efi_net.c      |  57 +++-
 lib/efi_loader/efi_watchdog.c |  58 ++++
 7 files changed, 763 insertions(+), 205 deletions(-)
 create mode 100644 lib/efi_loader/efi_watchdog.c

-- 
2.14.1

[U-Boot] [PATCH 01/23] efi_loader: allow return value in EFI_CALL

[Heinrich Schuchardt]

Macro EFI_CALL was introduced to call an UEFI function.
Unfortunately it does not support return values.
Most UEFI functions have a return value.

So let's rename EFI_CALL to EFI_CALL_VOID and introduce a
new EFI_CALL macro that supports return values.

Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_loader.h          | 17 +++++++++++++++--
 lib/efi_loader/efi_boottime.c |  3 ++-
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 1179234f68..6f71a6202b 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -41,9 +41,22 @@ const char *__efi_nesting_dec(void);
 	})
 
 /*
- * Callback into UEFI world from u-boot:
+ * Call non-void UEFI function from u-boot and retrieve return value:
  */
-#define EFI_CALL(exp) do { \
+#define EFI_CALL(exp) ({ \
+	debug("%sEFI: Call: %s\n", __efi_nesting_inc(), #exp); \
+	assert(__efi_exit_check()); \
+	typeof(exp) _r = exp; \
+	assert(__efi_entry_check()); \
+	debug("%sEFI: %lu returned by %s\n", __efi_nesting_dec(), \
+	      (unsigned long)((uintptr_t)_r & ~EFI_ERROR_MASK), #exp); \
+	_r; \
+})
+
+/*
+ * Call void UEFI function from u-boot:
+ */
+#define EFI_CALL_VOID(exp) do { \
 	debug("%sEFI: Call: %s\n", __efi_nesting_inc(), #exp); \
 	assert(__efi_exit_check()); \
 	exp; \
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index ea953dca82..ab26e2989b 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -158,7 +158,8 @@ void efi_signal_event(struct efi_event *event)
 		return;
 	event->signaled = 1;
 	if (event->type & EVT_NOTIFY_SIGNAL) {
-		EFI_CALL(event->notify_function(event, event->notify_context));
+		EFI_CALL_VOID(event->notify_function(event,
+						     event->notify_context));
 	}
 }
 
-- 
2.14.1

[U-Boot] [PATCH 02/23] efi_loader: notify when ExitBootServices is invoked

[Heinrich Schuchardt]

All events of type EVT_SIGNAL_EXIT_BOOT_SERVICES have to be
notified when ExitBootServices is invoked.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_boottime.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index ab26e2989b..b5538e0769 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -843,8 +843,17 @@ static void efi_exit_caches(void)
 static efi_status_t EFIAPI efi_exit_boot_services(void *image_handle,
 						  unsigned long map_key)
 {
+	int i;
+
 	EFI_ENTRY("%p, %ld", image_handle, map_key);
 
+	/* Notify that ExitBootServices is invoked. */
+	for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
+		if (efi_events[i].type != EVT_SIGNAL_EXIT_BOOT_SERVICES)
+			continue;
+		efi_signal_event(&efi_events[i]);
+	}
+
 	board_quiesce_devices();
 
 	/* Fix up caches for EFI payloads if necessary */
-- 
2.14.1

[U-Boot] [PATCH 03/23] efi_loader: support 16 protocols per efi_object

[Heinrich Schuchardt]

8 protocols per efi_object is insufficient for iPXE.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_loader.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 6f71a6202b..e8fb4fbb0a 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -99,8 +99,8 @@ struct efi_handler {
 struct efi_object {
 	/* Every UEFI object is part of a global object list */
 	struct list_head link;
-	/* We support up to 8 "protocols" an object can be accessed through */
-	struct efi_handler protocols[8];
+	/* We support up to 16 "protocols" an object can be accessed through */
+	struct efi_handler protocols[16];
 	/* The object spawner can either use this for data or as identifier */
 	void *handle;
 };
-- 
2.14.1

[U-Boot] [PATCH 04/23] efi_loader: rework efi_locate_handle

[Heinrich Schuchardt]

Check the parameters in efi_locate_handle.

Use list_for_each_entry instead of list_for_each.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_boottime.c | 42 +++++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index b5538e0769..570a5ea186 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -599,6 +599,7 @@ static int efi_search(enum efi_locate_search_type search_type,
 	case all_handles:
 		return 0;
 	case by_register_notify:
+		/* RegisterProtocolNotify is not implemented yet */
 		return -1;
 	case by_protocol:
 		for (i = 0; i < ARRAY_SIZE(efiobj->protocols); i++) {
@@ -617,16 +618,38 @@ static efi_status_t efi_locate_handle(
 			efi_guid_t *protocol, void *search_key,
 			unsigned long *buffer_size, efi_handle_t *buffer)
 {
-	struct list_head *lhandle;
+	struct efi_object *efiobj;
 	unsigned long size = 0;
 
+	/* Check parameters */
+	switch (search_type) {
+	case all_handles:
+		break;
+	case by_register_notify:
+		if (!search_key)
+			return EFI_INVALID_PARAMETER;
+		/* RegisterProtocolNotify is not implemented yet */
+		return EFI_UNSUPPORTED;
+	case by_protocol:
+		if (!protocol)
+			return EFI_INVALID_PARAMETER;
+		break;
+	default:
+		return EFI_INVALID_PARAMETER;
+	}
+
+	/*
+	 * efi_locate_handle_buffer uses this function for
+	 * the calculation of the necessary buffer size.
+	 * So do not require a buffer for buffersize == 0.
+	 */
+	if (!buffer_size || (*buffer_size && !buffer))
+		return EFI_INVALID_PARAMETER;
+
 	/* Count how much space we need */
-	list_for_each(lhandle, &efi_obj_list) {
-		struct efi_object *efiobj;
-		efiobj = list_entry(lhandle, struct efi_object, link);
-		if (!efi_search(search_type, protocol, search_key, efiobj)) {
+	list_for_each_entry(efiobj, &efi_obj_list, link) {
+		if (!efi_search(search_type, protocol, search_key, efiobj))
 			size += sizeof(void*);
-		}
 	}
 
 	if (*buffer_size < size) {
@@ -639,12 +662,9 @@ static efi_status_t efi_locate_handle(
 		return EFI_NOT_FOUND;
 
 	/* Then fill the array */
-	list_for_each(lhandle, &efi_obj_list) {
-		struct efi_object *efiobj;
-		efiobj = list_entry(lhandle, struct efi_object, link);
-		if (!efi_search(search_type, protocol, search_key, efiobj)) {
+	list_for_each_entry(efiobj, &efi_obj_list, link) {
+		if (!efi_search(search_type, protocol, search_key, efiobj))
 			*(buffer++) = efiobj->handle;
-		}
 	}
 
 	return EFI_SUCCESS;
-- 
2.14.1

[U-Boot] [PATCH 05/23] efi_loader: rework efi_search_obj

[Heinrich Schuchardt]

EFI_HANDLEs are used both in boottime and in runtime services.
efi_search_obj is a function that can be used to validate
handles. So let's make it accessible via efi_loader.h.

We can simplify the coding using list_for_each_entry.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_loader.h          | 2 ++
 lib/efi_loader/efi_boottime.c | 8 +++-----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index e8fb4fbb0a..193fca24ce 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -167,6 +167,8 @@ void efi_restore_gd(void);
 void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map);
 /* Call this to set the current device name */
 void efi_set_bootdev(const char *dev, const char *devnr, const char *path);
+/* Call this to validate a handle and find the EFI object for it */
+struct efi_object *efi_search_obj(void *handle);
 /* Call this to create an event */
 efi_status_t efi_create_event(uint32_t type, UINTN notify_tpl,
 			      void (EFIAPI *notify_function) (
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 570a5ea186..b643d299b9 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -822,13 +822,11 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
 	panic("EFI application exited");
 }
 
-static struct efi_object *efi_search_obj(void *handle)
+struct efi_object *efi_search_obj(void *handle)
 {
-	struct list_head *lhandle;
+	struct efi_object *efiobj;
 
-	list_for_each(lhandle, &efi_obj_list) {
-		struct efi_object *efiobj;
-		efiobj = list_entry(lhandle, struct efi_object, link);
+	list_for_each_entry(efiobj, &efi_obj_list, link) {
 		if (efiobj->handle == handle)
 			return efiobj;
 	}
-- 
2.14.1

[U-Boot] [PATCH 06/23] efi_loader: new function efi_search_protocol

[Heinrich Schuchardt]

In multiple functions we are searching for the protocol of a handle.
This patch provides a new function efi_search_protocol that we can
use to avoid duplicating code.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_boottime.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index b643d299b9..9dae02daca 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -453,6 +453,31 @@ static efi_status_t EFIAPI efi_check_event(struct efi_event *event)
 	return EFI_EXIT(EFI_INVALID_PARAMETER);
 }
 
+static efi_status_t efi_search_protocol(void *handle, efi_guid_t *protocol_guid,
+					struct efi_handler **handler)
+{
+	struct efi_object *efiobj;
+	size_t i;
+	struct efi_handler *protocol;
+
+	if (!handle || !protocol_guid)
+		return EFI_INVALID_PARAMETER;
+	efiobj = efi_search_obj(handle);
+	if (!efiobj)
+		return EFI_INVALID_PARAMETER;
+	for (i = 0; i < ARRAY_SIZE(efiobj->protocols); i++) {
+		protocol = &efiobj->protocols[i];
+		if (!protocol->guid)
+			continue;
+		if (!guidcmp(protocol->guid, protocol_guid)) {
+			if (handler)
+				*handler = protocol;
+			return EFI_SUCCESS;
+		}
+	}
+	return EFI_NOT_FOUND;
+}
+
 static efi_status_t EFIAPI efi_install_protocol_interface(void **handle,
 			efi_guid_t *protocol, int protocol_interface_type,
 			void *protocol_interface)
-- 
2.14.1

[U-Boot] [PATCH 07/23] efi_loader: simplify efi_install_protocol_interface

[Heinrich Schuchardt]

Use function efi_search_obj and efi_search_protocol
to simplify the coding.

Do away with efi_install_protocol_interface_ext.
We can use EFI_CALL for internal usage.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_boottime.c | 76 ++++++++++++++++++-------------------------
 1 file changed, 31 insertions(+), 45 deletions(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 9dae02daca..96cb1fa410 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -482,9 +482,12 @@ static efi_status_t EFIAPI efi_install_protocol_interface(void **handle,
 			efi_guid_t *protocol, int protocol_interface_type,
 			void *protocol_interface)
 {
-	struct list_head *lhandle;
 	int i;
 	efi_status_t r;
+	struct efi_object *efiobj;
+
+	EFI_ENTRY("%p, %p, %d, %p", handle, protocol, protocol_interface_type,
+		  protocol_interface);
 
 	if (!handle || !protocol ||
 	    protocol_interface_type != EFI_NATIVE_INTERFACE) {
@@ -497,54 +500,36 @@ static efi_status_t EFIAPI efi_install_protocol_interface(void **handle,
 		r = EFI_OUT_OF_RESOURCES;
 		goto out;
 	}
+
 	/* Find object. */
-	list_for_each(lhandle, &efi_obj_list) {
-		struct efi_object *efiobj;
-		efiobj = list_entry(lhandle, struct efi_object, link);
+	efiobj = efi_search_obj(*handle);
+	if (!efiobj) {
+		r = EFI_INVALID_PARAMETER;
+		goto out;
+	}
 
-		if (efiobj->handle != *handle)
-			continue;
-		/* Check if protocol is already installed on the handle. */
-		for (i = 0; i < ARRAY_SIZE(efiobj->protocols); i++) {
-			struct efi_handler *handler = &efiobj->protocols[i];
+	/* Check if protocol is already installed on the handle. */
+	r = efi_search_protocol(*handle, protocol, NULL);
+	if (r == EFI_SUCCESS) {
+		r = EFI_INVALID_PARAMETER;
+		goto out;
+	}
 
-			if (!handler->guid)
-				continue;
-			if (!guidcmp(handler->guid, protocol)) {
-				r = EFI_INVALID_PARAMETER;
-				goto out;
-			}
-		}
-		/* Install protocol in first empty slot. */
-		for (i = 0; i < ARRAY_SIZE(efiobj->protocols); i++) {
-			struct efi_handler *handler = &efiobj->protocols[i];
+	/* Install protocol in first empty slot. */
+	for (i = 0; i < ARRAY_SIZE(efiobj->protocols); i++) {
+		struct efi_handler *handler = &efiobj->protocols[i];
 
-			if (handler->guid)
-				continue;
+		if (handler->guid)
+			continue;
 
-			handler->guid = protocol;
-			handler->protocol_interface = protocol_interface;
-			r = EFI_SUCCESS;
-			goto out;
-		}
-		r = EFI_OUT_OF_RESOURCES;
+		handler->guid = protocol;
+		handler->protocol_interface = protocol_interface;
+		r = EFI_SUCCESS;
 		goto out;
 	}
-	r = EFI_INVALID_PARAMETER;
+	r = EFI_OUT_OF_RESOURCES;
 out:
-	return r;
-}
-
-static efi_status_t EFIAPI efi_install_protocol_interface_ext(void **handle,
-			efi_guid_t *protocol, int protocol_interface_type,
-			void *protocol_interface)
-{
-	EFI_ENTRY("%p, %p, %d, %p", handle, protocol, protocol_interface_type,
-		  protocol_interface);
-
-	return EFI_EXIT(efi_install_protocol_interface(handle, protocol,
-						       protocol_interface_type,
-						       protocol_interface));
+	return EFI_EXIT(r);
 }
 
 static efi_status_t EFIAPI efi_reinstall_protocol_interface(void *handle,
@@ -1115,9 +1100,10 @@ static efi_status_t EFIAPI efi_install_multiple_protocol_interfaces(
 		if (!protocol)
 			break;
 		protocol_interface = va_arg(argptr, void*);
-		r = efi_install_protocol_interface(handle, protocol,
-						   EFI_NATIVE_INTERFACE,
-						   protocol_interface);
+		r = EFI_CALL(efi_install_protocol_interface(
+						handle, protocol,
+						EFI_NATIVE_INTERFACE,
+						protocol_interface));
 		if (r != EFI_SUCCESS)
 			break;
 		i++;
@@ -1263,7 +1249,7 @@ static const struct efi_boot_services efi_boot_services = {
 	.signal_event = efi_signal_event_ext,
 	.close_event = efi_close_event,
 	.check_event = efi_check_event,
-	.install_protocol_interface = efi_install_protocol_interface_ext,
+	.install_protocol_interface = efi_install_protocol_interface,
 	.reinstall_protocol_interface = efi_reinstall_protocol_interface,
 	.uninstall_protocol_interface = efi_uninstall_protocol_interface_ext,
 	.handle_protocol = efi_handle_protocol,
-- 
2.14.1

[U-Boot] [PATCH 08/23] efi_loader: allow creating new handles

[Heinrich Schuchardt]

In efi_install_protocol_interface support creating
a new handle.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_boottime.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 96cb1fa410..9f8d64659f 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -238,6 +238,23 @@ static efi_status_t EFIAPI efi_free_pool_ext(void *buffer)
 	return EFI_EXIT(r);
 }
 
+static efi_status_t efi_create_handle(void **handle)
+{
+	struct efi_object *obj;
+	efi_status_t r;
+
+	r = efi_allocate_pool(EFI_ALLOCATE_ANY_PAGES,
+			      sizeof(struct efi_object),
+			      (void **)&obj);
+	if (r != EFI_SUCCESS)
+		return r;
+	memset(obj, 0, sizeof(struct efi_object));
+	obj->handle = obj;
+	list_add_tail(&obj->link, &efi_obj_list);
+	*handle = obj;
+	return r;
+}
+
 /*
  * Our event capabilities are very limited. Only a small limited
  * number of events is allowed to coexist.
@@ -497,8 +514,9 @@ static efi_status_t EFIAPI efi_install_protocol_interface(void **handle,
 
 	/* Create new handle if requested. */
 	if (!*handle) {
-		r = EFI_OUT_OF_RESOURCES;
-		goto out;
+		r = efi_create_handle(handle);
+		if (r != EFI_SUCCESS)
+			goto out;
 	}
 
 	/* Find object. */
-- 
2.14.1

[U-Boot] [PATCH 09/23] efi_loader: simplify efi_uninstall_protocol_interface

[Heinrich Schuchardt]

Use function efi_search_obj and efi_search_protocol
to simplify the coding.

Do away with efi_uninstall_protocol_interface_ext.
We can use EFI_CALL for internal usage.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_boottime.c | 56 ++++++++++++++-----------------------------
 1 file changed, 18 insertions(+), 38 deletions(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 9f8d64659f..a483b827cd 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -562,51 +562,31 @@ static efi_status_t EFIAPI efi_reinstall_protocol_interface(void *handle,
 static efi_status_t EFIAPI efi_uninstall_protocol_interface(void *handle,
 			efi_guid_t *protocol, void *protocol_interface)
 {
-	struct list_head *lhandle;
-	int i;
-	efi_status_t r = EFI_NOT_FOUND;
+	struct efi_handler *handler;
+	efi_status_t r;
+
+	EFI_ENTRY("%p, %p, %p", handle, protocol, protocol_interface);
 
 	if (!handle || !protocol) {
 		r = EFI_INVALID_PARAMETER;
 		goto out;
 	}
 
-	list_for_each(lhandle, &efi_obj_list) {
-		struct efi_object *efiobj;
-		efiobj = list_entry(lhandle, struct efi_object, link);
-
-		if (efiobj->handle != handle)
-			continue;
-
-		for (i = 0; i < ARRAY_SIZE(efiobj->protocols); i++) {
-			struct efi_handler *handler = &efiobj->protocols[i];
-			const efi_guid_t *hprotocol = handler->guid;
+	/* Find the protocol on the handle */
+	r = efi_search_protocol(handle, protocol, &handler);
+	if (r != EFI_SUCCESS)
+		goto out;
 
-			if (!hprotocol)
-				continue;
-			if (!guidcmp(hprotocol, protocol)) {
-				if (handler->protocol_interface) {
-					r = EFI_ACCESS_DENIED;
-				} else {
-					handler->guid = 0;
-					r = EFI_SUCCESS;
-				}
-				goto out;
-			}
-		}
+	if (handler->protocol_interface) {
+		/* Disconnect controllers */
+		r =  EFI_ACCESS_DENIED;
+	} else {
+		handler->guid = 0;
+		r = EFI_SUCCESS;
 	}
 
 out:
-	return r;
-}
-
-static efi_status_t EFIAPI efi_uninstall_protocol_interface_ext(void *handle,
-			efi_guid_t *protocol, void *protocol_interface)
-{
-	EFI_ENTRY("%p, %p, %p", handle, protocol, protocol_interface);
-
-	return EFI_EXIT(efi_uninstall_protocol_interface(handle, protocol,
-							 protocol_interface));
+	return EFI_EXIT(r);
 }
 
 static efi_status_t EFIAPI efi_register_protocol_notify(efi_guid_t *protocol,
@@ -1135,8 +1115,8 @@ static efi_status_t EFIAPI efi_install_multiple_protocol_interfaces(
 	for (; i; --i) {
 		protocol = va_arg(argptr, efi_guid_t*);
 		protocol_interface = va_arg(argptr, void*);
-		efi_uninstall_protocol_interface(handle, protocol,
-						 protocol_interface);
+		EFI_CALL(efi_uninstall_protocol_interface(handle, protocol,
+							  protocol_interface));
 	}
 	va_end(argptr);
 
@@ -1269,7 +1249,7 @@ static const struct efi_boot_services efi_boot_services = {
 	.check_event = efi_check_event,
 	.install_protocol_interface = efi_install_protocol_interface,
 	.reinstall_protocol_interface = efi_reinstall_protocol_interface,
-	.uninstall_protocol_interface = efi_uninstall_protocol_interface_ext,
+	.uninstall_protocol_interface = efi_uninstall_protocol_interface,
 	.handle_protocol = efi_handle_protocol,
 	.reserved = NULL,
 	.register_protocol_notify = efi_register_protocol_notify,
-- 
2.14.1

[U-Boot] [PATCH 10/23] efi_loader: open_info in OpenProtocol

[Heinrich Schuchardt]

efi_open_protocol and close_protocol have to keep track of
opened protocols.

So we add an array open_info to each protocol of each handle.

OpenProtocol has enter the agent and controller handle
information into this array.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_loader.h          |   1 +
 lib/efi_loader/efi_boottime.c | 130 +++++++++++++++++++++++++++++++-----------
 2 files changed, 99 insertions(+), 32 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 193fca24ce..2c3360534b 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -87,6 +87,7 @@ extern unsigned int __efi_runtime_rel_start, __efi_runtime_rel_stop;
 struct efi_handler {
 	const efi_guid_t *guid;
 	void *protocol_interface;
+	struct efi_open_protocol_info_entry open_info[4];
 };
 
 /*
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index a483b827cd..294bc1f138 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1152,24 +1152,111 @@ static void EFIAPI efi_set_mem(void *buffer, unsigned long size, uint8_t value)
 	memset(buffer, value, size);
 }
 
+static efi_status_t efi_protocol_open(
+			struct efi_handler *protocol,
+			void **protocol_interface, void *agent_handle,
+			void *controller_handle, uint32_t attributes)
+{
+	bool opened_exclusive = false;
+	bool opened_by_driver = false;
+	int i;
+	struct efi_open_protocol_info_entry *open_info;
+	struct efi_open_protocol_info_entry *match = NULL;
+
+	if (attributes !=
+	    EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
+		*protocol_interface = NULL;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(protocol->open_info); ++i) {
+		open_info = &protocol->open_info[i];
+
+		if (!open_info->open_count)
+			continue;
+		if (open_info->agent_handle == agent_handle) {
+			if ((attributes & EFI_OPEN_PROTOCOL_BY_DRIVER) &&
+			    (open_info->attributes == attributes))
+				return EFI_ALREADY_STARTED;
+			if (open_info->controller_handle == controller_handle)
+				match = open_info;
+		}
+		if (open_info->attributes & EFI_OPEN_PROTOCOL_EXCLUSIVE)
+			opened_exclusive = true;
+	}
+
+	if (attributes &
+	    (EFI_OPEN_PROTOCOL_EXCLUSIVE | EFI_OPEN_PROTOCOL_BY_DRIVER) &&
+	    opened_exclusive)
+		return EFI_ACCESS_DENIED;
+
+	if (attributes & EFI_OPEN_PROTOCOL_EXCLUSIVE) {
+		for (i = 0; i < ARRAY_SIZE(protocol->open_info); ++i) {
+			open_info = &protocol->open_info[i];
+
+			if (!open_info->open_count)
+				continue;
+			if (open_info->attributes ==
+					EFI_OPEN_PROTOCOL_BY_DRIVER)
+				EFI_CALL(efi_disconnect_controller(
+						open_info->controller_handle,
+						open_info->agent_handle,
+						NULL));
+		}
+		opened_by_driver = false;
+		for (i = 0; i < ARRAY_SIZE(protocol->open_info); ++i) {
+			open_info = &protocol->open_info[i];
+
+			if (!open_info->open_count)
+				continue;
+			if (open_info->attributes & EFI_OPEN_PROTOCOL_BY_DRIVER)
+				opened_by_driver = true;
+		}
+		if (opened_by_driver)
+			return EFI_ACCESS_DENIED;
+		if (match && !match->open_count)
+			match = NULL;
+	}
+
+	/*
+	 * Find an empty slot.
+	 */
+	if (!match) {
+		for (i = 0; i < ARRAY_SIZE(protocol->open_info); ++i) {
+			open_info = &protocol->open_info[i];
+
+			if (!open_info->open_count) {
+				match = open_info;
+				break;
+			}
+		}
+	}
+	if (!match)
+		return EFI_OUT_OF_RESOURCES;
+
+	match->agent_handle = agent_handle;
+	match->controller_handle = controller_handle;
+	match->attributes = attributes;
+	match->open_count++;
+	*protocol_interface = protocol->protocol_interface;
+
+	return EFI_SUCCESS;
+}
+
 static efi_status_t EFIAPI efi_open_protocol(
 			void *handle, efi_guid_t *protocol,
 			void **protocol_interface, void *agent_handle,
 			void *controller_handle, uint32_t attributes)
 {
-	struct list_head *lhandle;
-	int i;
+	struct efi_handler *handler;
 	efi_status_t r = EFI_INVALID_PARAMETER;
 
 	EFI_ENTRY("%p, %p, %p, %p, %p, 0x%x", handle, protocol,
 		  protocol_interface, agent_handle, controller_handle,
 		  attributes);
 
-	if (!handle || !protocol ||
-	    (!protocol_interface && attributes !=
-	     EFI_OPEN_PROTOCOL_TEST_PROTOCOL)) {
+	if (!protocol_interface && attributes !=
+	    EFI_OPEN_PROTOCOL_TEST_PROTOCOL)
 		goto out;
-	}
 
 	switch (attributes) {
 	case EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL:
@@ -1191,33 +1278,12 @@ static efi_status_t EFIAPI efi_open_protocol(
 		goto out;
 	}
 
-	list_for_each(lhandle, &efi_obj_list) {
-		struct efi_object *efiobj;
-		efiobj = list_entry(lhandle, struct efi_object, link);
-
-		if (efiobj->handle != handle)
-			continue;
-
-		for (i = 0; i < ARRAY_SIZE(efiobj->protocols); i++) {
-			struct efi_handler *handler = &efiobj->protocols[i];
-			const efi_guid_t *hprotocol = handler->guid;
-			if (!hprotocol)
-				continue;
-			if (!guidcmp(hprotocol, protocol)) {
-				if (attributes !=
-				    EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
-					*protocol_interface =
-						handler->protocol_interface;
-				}
-				r = EFI_SUCCESS;
-				goto out;
-			}
-		}
-		goto unsupported;
-	}
+	r = efi_search_protocol(handle, protocol, &handler);
+	if (r != EFI_SUCCESS)
+		goto out;
 
-unsupported:
-	r = EFI_UNSUPPORTED;
+	r = efi_protocol_open(handler, protocol_interface, agent_handle,
+			      controller_handle, attributes);
 out:
 	return EFI_EXIT(r);
 }
-- 
2.14.1

[U-Boot] [PATCH 11/23] efi_loader: open_info in CloseProtocol

[Heinrich Schuchardt]

efi_open_protocol and efi_close_protocol have to keep track of
opened protocols.

efi_close_protocol has to mark the appropriate entry as
empty.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_boottime.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 294bc1f138..c9aec597a2 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -944,9 +944,40 @@ static efi_status_t EFIAPI efi_close_protocol(void *handle,
 					      void *agent_handle,
 					      void *controller_handle)
 {
+	struct efi_handler *handler;
+	size_t i;
+	struct efi_open_protocol_info_entry *open_info;
+	efi_status_t r;
+
 	EFI_ENTRY("%p, %p, %p, %p", handle, protocol, agent_handle,
 		  controller_handle);
-	return EFI_EXIT(EFI_NOT_FOUND);
+
+	if (!agent_handle) {
+		r = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+
+	r = efi_search_protocol(handle, protocol, &handler);
+	if (r != EFI_SUCCESS)
+		goto out;
+
+	for (i = 0; i < ARRAY_SIZE(handler->open_info); ++i) {
+		open_info = &handler->open_info[i];
+
+		if (!open_info->open_count)
+			continue;
+
+		if (open_info->agent_handle == agent_handle &&
+		    open_info->controller_handle ==
+		    controller_handle) {
+			open_info->open_count--;
+			r = EFI_SUCCESS;
+			goto out;
+		}
+	}
+	r = EFI_NOT_FOUND;
+out:
+	return EFI_EXIT(r);
 }
 
 static efi_status_t EFIAPI efi_open_protocol_information(efi_handle_t handle,
-- 
2.14.1

[U-Boot] [PATCH 12/23] efi_loader: implement OpenProtocolInformation

[Heinrich Schuchardt]

efi_open_protocol_information provides the agent and controller
handles as well as the attributes and open count of an protocol
on a handle.

Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_boottime.c | 55 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index c9aec597a2..23b8894e73 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -985,9 +985,62 @@ static efi_status_t EFIAPI efi_open_protocol_information(efi_handle_t handle,
 			struct efi_open_protocol_info_entry **entry_buffer,
 			unsigned long *entry_count)
 {
+	unsigned long buffer_size;
+	unsigned long count;
+	struct efi_handler *handler;
+	size_t i;
+	efi_status_t r;
+
 	EFI_ENTRY("%p, %p, %p, %p", handle, protocol, entry_buffer,
 		  entry_count);
-	return EFI_EXIT(EFI_NOT_FOUND);
+
+	/* Check parameters */
+	if (!handle || !protocol || !entry_buffer) {
+		r = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+
+	/* Find the protocol */
+	r = efi_search_protocol(handle, protocol, &handler);
+	if (r != EFI_SUCCESS)
+		goto out;
+
+	*entry_buffer = NULL;
+
+	/* Count entries */
+	count = 0;
+	for (i = 0; i < ARRAY_SIZE(handler->open_info); ++i) {
+		struct efi_open_protocol_info_entry *open_info =
+			&handler->open_info[i];
+
+		if (open_info->open_count)
+			++count;
+	}
+	*entry_count = count;
+	if (!count) {
+		r = EFI_SUCCESS;
+		goto out;
+	}
+
+	/* Copy entries */
+	buffer_size = count * sizeof(struct efi_open_protocol_info_entry);
+	r = efi_allocate_pool(EFI_ALLOCATE_ANY_PAGES, buffer_size,
+			      (void **)entry_buffer);
+	if (r != EFI_SUCCESS)
+		goto out;
+	count = 0;
+	for (i = 0; i < ARRAY_SIZE(handler->open_info); ++i) {
+		struct efi_open_protocol_info_entry *open_info =
+			&handler->open_info[i];
+
+		if (!open_info->open_count)
+			continue;
+		(*entry_buffer)[count] = *open_info;
+		++count;
+	}
+
+out:
+	return EFI_EXIT(r);
 }
 
 static efi_status_t EFIAPI efi_protocols_per_handle(void *handle,
-- 
2.14.1

[U-Boot] [PATCH 13/23] efi_loader: non-static efi_open_protocol, efi_close_protocol

[Heinrich Schuchardt]

We need efi_open_protocol and efi_close_protocol for
implementing other functions. So they shouldn't be static.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_loader.h          | 9 +++++++++
 lib/efi_loader/efi_boottime.c | 9 ++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 2c3360534b..2a98bf66b8 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -181,6 +181,15 @@ efi_status_t efi_set_timer(struct efi_event *event, enum efi_timer_delay type,
 			   uint64_t trigger_time);
 /* Call this to signal an event */
 void efi_signal_event(struct efi_event *event);
+/* Call this with EFI_CALL to close a protocol */
+efi_status_t EFIAPI efi_close_protocol(void *handle, efi_guid_t *protocol,
+				       void *agent_handle,
+				       void *controller_handle);
+/* Call this with EFI_CALL to open a protocol */
+efi_status_t EFIAPI efi_open_protocol(
+			void *handle, efi_guid_t *protocol,
+			void **protocol_interface, void *agent_handle,
+			void *controller_handle, uint32_t attributes);
 
 /* Generic EFI memory allocator, call this to get memory */
 void *efi_alloc(uint64_t len, int memory_type);
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 23b8894e73..ad8733d3e5 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -939,10 +939,9 @@ static efi_status_t EFIAPI efi_disconnect_controller(void *controller_handle,
 	return EFI_EXIT(EFI_INVALID_PARAMETER);
 }
 
-static efi_status_t EFIAPI efi_close_protocol(void *handle,
-					      efi_guid_t *protocol,
-					      void *agent_handle,
-					      void *controller_handle)
+efi_status_t EFIAPI efi_close_protocol(void *handle, efi_guid_t *protocol,
+				       void *agent_handle,
+				       void *controller_handle)
 {
 	struct efi_handler *handler;
 	size_t i;
@@ -1326,7 +1325,7 @@ static efi_status_t efi_protocol_open(
 	return EFI_SUCCESS;
 }
 
-static efi_status_t EFIAPI efi_open_protocol(
+efi_status_t EFIAPI efi_open_protocol(
 			void *handle, efi_guid_t *protocol,
 			void **protocol_interface, void *agent_handle,
 			void *controller_handle, uint32_t attributes)
-- 
2.14.1

[U-Boot] [PATCH 14/23] efi_loader: pass GUIDs as const efi_guid_t *

[Heinrich Schuchardt]

We need to call some boottime services internally.
Our GUIDs are stored as const efi_guid_t *.

The boottime services never change GUIDs.
So we can define the parameters as const efi_guid_t *.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_api.h             | 44 +++++++++++++++++++++----------------------
 include/efi_loader.h          |  4 ++--
 lib/efi_loader/efi_boottime.c | 37 ++++++++++++++++++------------------
 3 files changed, 43 insertions(+), 42 deletions(-)

diff --git a/include/efi_api.h b/include/efi_api.h
index ec1b321e8e..8efc8dfab8 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -74,25 +74,25 @@ struct efi_boot_services {
 	efi_status_t (EFIAPI *close_event)(struct efi_event *event);
 	efi_status_t (EFIAPI *check_event)(struct efi_event *event);
 #define EFI_NATIVE_INTERFACE	0x00000000
-	efi_status_t (EFIAPI *install_protocol_interface)(
-			void **handle, efi_guid_t *protocol,
+	efi_status_t (EFIAPI * install_protocol_interface)(
+			void **handle, const efi_guid_t *protocol,
 			int protocol_interface_type, void *protocol_interface);
-	efi_status_t (EFIAPI *reinstall_protocol_interface)(
-			void *handle, efi_guid_t *protocol,
+	efi_status_t (EFIAPI * reinstall_protocol_interface)(
+			void *handle, const efi_guid_t *protocol,
 			void *old_interface, void *new_interface);
-	efi_status_t (EFIAPI *uninstall_protocol_interface)(void *handle,
-			efi_guid_t *protocol, void *protocol_interface);
-	efi_status_t (EFIAPI *handle_protocol)(efi_handle_t, efi_guid_t *,
-					       void **);
+	efi_status_t (EFIAPI * uninstall_protocol_interface)(void *handle,
+			const efi_guid_t *protocol, void *protocol_interface);
+	efi_status_t (EFIAPI * handle_protocol)(efi_handle_t,
+						const efi_guid_t *, void **);
 	void *reserved;
-	efi_status_t (EFIAPI *register_protocol_notify)(
-			efi_guid_t *protocol, struct efi_event *event,
+	efi_status_t (EFIAPI * register_protocol_notify)(
+			const efi_guid_t *protocol, struct efi_event *event,
 			void **registration);
-	efi_status_t (EFIAPI *locate_handle)(
+	efi_status_t (EFIAPI * locate_handle)(
 			enum efi_locate_search_type search_type,
-			efi_guid_t *protocol, void *search_key,
+			const efi_guid_t *protocol, void *search_key,
 			unsigned long *buffer_size, efi_handle_t *buffer);
-	efi_status_t (EFIAPI *locate_device_path)(efi_guid_t *protocol,
+	efi_status_t (EFIAPI * locate_device_path)(const efi_guid_t *protocol,
 			struct efi_device_path **device_path,
 			efi_handle_t *device);
 	efi_status_t (EFIAPI *install_configuration_table)(
@@ -128,25 +128,25 @@ struct efi_boot_services {
 #define EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER 0x00000008
 #define EFI_OPEN_PROTOCOL_BY_DRIVER           0x00000010
 #define EFI_OPEN_PROTOCOL_EXCLUSIVE           0x00000020
-	efi_status_t (EFIAPI *open_protocol)(efi_handle_t handle,
-			efi_guid_t *protocol, void **interface,
+	efi_status_t (EFIAPI * open_protocol)(efi_handle_t handle,
+			const efi_guid_t *protocol, void **interface,
 			efi_handle_t agent_handle,
 			efi_handle_t controller_handle, u32 attributes);
-	efi_status_t (EFIAPI *close_protocol)(void *handle,
-			efi_guid_t *protocol, void *agent_handle,
+	efi_status_t (EFIAPI * close_protocol)(void *handle,
+			const efi_guid_t *protocol, void *agent_handle,
 			void *controller_handle);
-	efi_status_t(EFIAPI *open_protocol_information)(efi_handle_t handle,
-			efi_guid_t *protocol,
+	efi_status_t(EFIAPI * open_protocol_information)(efi_handle_t handle,
+			const efi_guid_t *protocol,
 			struct efi_open_protocol_info_entry **entry_buffer,
 			unsigned long *entry_count);
 	efi_status_t (EFIAPI *protocols_per_handle)(efi_handle_t handle,
 			efi_guid_t ***protocol_buffer,
 			unsigned long *protocols_buffer_count);
-	efi_status_t (EFIAPI *locate_handle_buffer) (
+	efi_status_t (EFIAPI * locate_handle_buffer) (
 			enum efi_locate_search_type search_type,
-			efi_guid_t *protocol, void *search_key,
+			const efi_guid_t *protocol, void *search_key,
 			unsigned long *no_handles, efi_handle_t **buffer);
-	efi_status_t (EFIAPI *locate_protocol)(efi_guid_t *protocol,
+	efi_status_t (EFIAPI * locate_protocol)(const efi_guid_t *protocol,
 			void *registration, void **protocol_interface);
 	efi_status_t (EFIAPI *install_multiple_protocol_interfaces)(
 			void **handle, ...);
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 2a98bf66b8..9c68246c7c 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -182,12 +182,12 @@ efi_status_t efi_set_timer(struct efi_event *event, enum efi_timer_delay type,
 /* Call this to signal an event */
 void efi_signal_event(struct efi_event *event);
 /* Call this with EFI_CALL to close a protocol */
-efi_status_t EFIAPI efi_close_protocol(void *handle, efi_guid_t *protocol,
+efi_status_t EFIAPI efi_close_protocol(void *handle, const efi_guid_t *protocol,
 				       void *agent_handle,
 				       void *controller_handle);
 /* Call this with EFI_CALL to open a protocol */
 efi_status_t EFIAPI efi_open_protocol(
-			void *handle, efi_guid_t *protocol,
+			void *handle, const 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 ad8733d3e5..5a73ea5cd0 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -470,7 +470,8 @@ static efi_status_t EFIAPI efi_check_event(struct efi_event *event)
 	return EFI_EXIT(EFI_INVALID_PARAMETER);
 }
 
-static efi_status_t efi_search_protocol(void *handle, efi_guid_t *protocol_guid,
+static efi_status_t efi_search_protocol(void *handle,
+					const efi_guid_t *protocol_guid,
 					struct efi_handler **handler)
 {
 	struct efi_object *efiobj;
@@ -496,7 +497,7 @@ static efi_status_t efi_search_protocol(void *handle, efi_guid_t *protocol_guid,
 }
 
 static efi_status_t EFIAPI efi_install_protocol_interface(void **handle,
-			efi_guid_t *protocol, int protocol_interface_type,
+			const efi_guid_t *protocol, int protocol_interface_type,
 			void *protocol_interface)
 {
 	int i;
@@ -551,7 +552,7 @@ out:
 }
 
 static efi_status_t EFIAPI efi_reinstall_protocol_interface(void *handle,
-			efi_guid_t *protocol, void *old_interface,
+			const efi_guid_t *protocol, void *old_interface,
 			void *new_interface)
 {
 	EFI_ENTRY("%p, %p, %p, %p", handle, protocol, old_interface,
@@ -560,7 +561,7 @@ static efi_status_t EFIAPI efi_reinstall_protocol_interface(void *handle,
 }
 
 static efi_status_t EFIAPI efi_uninstall_protocol_interface(void *handle,
-			efi_guid_t *protocol, void *protocol_interface)
+			const efi_guid_t *protocol, void *protocol_interface)
 {
 	struct efi_handler *handler;
 	efi_status_t r;
@@ -589,16 +590,16 @@ out:
 	return EFI_EXIT(r);
 }
 
-static efi_status_t EFIAPI efi_register_protocol_notify(efi_guid_t *protocol,
-							struct efi_event *event,
-							void **registration)
+static efi_status_t EFIAPI efi_register_protocol_notify(
+			const efi_guid_t *protocol, struct efi_event *event,
+			void **registration)
 {
 	EFI_ENTRY("%p, %p, %p", protocol, event, registration);
 	return EFI_EXIT(EFI_OUT_OF_RESOURCES);
 }
 
 static int efi_search(enum efi_locate_search_type search_type,
-		      efi_guid_t *protocol, void *search_key,
+		      const efi_guid_t *protocol, void *search_key,
 		      struct efi_object *efiobj)
 {
 	int i;
@@ -623,7 +624,7 @@ static int efi_search(enum efi_locate_search_type search_type,
 
 static efi_status_t efi_locate_handle(
 			enum efi_locate_search_type search_type,
-			efi_guid_t *protocol, void *search_key,
+			const efi_guid_t *protocol, void *search_key,
 			unsigned long *buffer_size, efi_handle_t *buffer)
 {
 	struct efi_object *efiobj;
@@ -680,7 +681,7 @@ static efi_status_t efi_locate_handle(
 
 static efi_status_t EFIAPI efi_locate_handle_ext(
 			enum efi_locate_search_type search_type,
-			efi_guid_t *protocol, void *search_key,
+			const efi_guid_t *protocol, void *search_key,
 			unsigned long *buffer_size, efi_handle_t *buffer)
 {
 	EFI_ENTRY("%d, %p, %p, %p, %p", search_type, protocol, search_key,
@@ -690,7 +691,7 @@ static efi_status_t EFIAPI efi_locate_handle_ext(
 			buffer_size, buffer));
 }
 
-static efi_status_t EFIAPI efi_locate_device_path(efi_guid_t *protocol,
+static efi_status_t EFIAPI efi_locate_device_path(const efi_guid_t *protocol,
 			struct efi_device_path **device_path,
 			efi_handle_t *device)
 {
@@ -939,7 +940,7 @@ static efi_status_t EFIAPI efi_disconnect_controller(void *controller_handle,
 	return EFI_EXIT(EFI_INVALID_PARAMETER);
 }
 
-efi_status_t EFIAPI efi_close_protocol(void *handle, efi_guid_t *protocol,
+efi_status_t EFIAPI efi_close_protocol(void *handle, const efi_guid_t *protocol,
 				       void *agent_handle,
 				       void *controller_handle)
 {
@@ -980,7 +981,7 @@ out:
 }
 
 static efi_status_t EFIAPI efi_open_protocol_information(efi_handle_t handle,
-			efi_guid_t *protocol,
+			const efi_guid_t *protocol,
 			struct efi_open_protocol_info_entry **entry_buffer,
 			unsigned long *entry_count)
 {
@@ -1097,7 +1098,7 @@ static efi_status_t EFIAPI efi_protocols_per_handle(void *handle,
 
 static efi_status_t EFIAPI efi_locate_handle_buffer(
 			enum efi_locate_search_type search_type,
-			efi_guid_t *protocol, void *search_key,
+			const efi_guid_t *protocol, void *search_key,
 			unsigned long *no_handles, efi_handle_t **buffer)
 {
 	efi_status_t r;
@@ -1128,7 +1129,7 @@ out:
 	return EFI_EXIT(r);
 }
 
-static efi_status_t EFIAPI efi_locate_protocol(efi_guid_t *protocol,
+static efi_status_t EFIAPI efi_locate_protocol(const efi_guid_t *protocol,
 					       void *registration,
 					       void **protocol_interface)
 {
@@ -1167,7 +1168,7 @@ static efi_status_t EFIAPI efi_install_multiple_protocol_interfaces(
 	EFI_ENTRY("%p", handle);
 
 	va_list argptr;
-	efi_guid_t *protocol;
+	const efi_guid_t *protocol;
 	void *protocol_interface;
 	efi_status_t r = EFI_SUCCESS;
 	int i = 0;
@@ -1326,7 +1327,7 @@ static efi_status_t efi_protocol_open(
 }
 
 efi_status_t EFIAPI efi_open_protocol(
-			void *handle, efi_guid_t *protocol,
+			void *handle, const efi_guid_t *protocol,
 			void **protocol_interface, void *agent_handle,
 			void *controller_handle, uint32_t attributes)
 {
@@ -1372,7 +1373,7 @@ out:
 }
 
 static efi_status_t EFIAPI efi_handle_protocol(void *handle,
-					       efi_guid_t *protocol,
+					       const efi_guid_t *protocol,
 					       void **protocol_interface)
 {
 	return efi_open_protocol(handle, protocol, protocol_interface, NULL,
-- 
2.14.1

[U-Boot] [PATCH 15/23] efi_loader: implement ConnectController

[Heinrich Schuchardt]

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_api.h             |  22 ++++++++
 include/efi_loader.h          |   1 +
 lib/efi_loader/efi_boottime.c | 119 +++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 141 insertions(+), 1 deletion(-)

diff --git a/include/efi_api.h b/include/efi_api.h
index 8efc8dfab8..b2838125d7 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -609,4 +609,26 @@ struct efi_pxe {
 	struct efi_pxe_mode *mode;
 };
 
+#define EFI_DRIVER_BINDING_PROTOCOL_GUID \
+	EFI_GUID(0x18a031ab, 0xb443, 0x4d1a,\
+		 0xa5, 0xc0, 0x0c, 0x09, 0x26, 0x1e, 0x9f, 0x71)
+struct efi_driver_binding_protocol {
+	efi_status_t (EFIAPI * supported)(
+			struct efi_driver_binding_protocol *this,
+			efi_handle_t controller_handle,
+			struct efi_device_path *remaining_device_path);
+	efi_status_t (EFIAPI * start)(
+			struct efi_driver_binding_protocol *this,
+			efi_handle_t controller_handle,
+			struct efi_device_path *remaining_device_path);
+	efi_status_t (EFIAPI * stop)(
+			struct efi_driver_binding_protocol *this,
+			efi_handle_t controller_handle,
+			UINTN number_of_children,
+			efi_handle_t child_handle_buffer);
+	u32 version;
+	efi_handle_t image_handle;
+	efi_handle_t driver_binding_handle;
+};
+
 #endif
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 9c68246c7c..f9f33e1d01 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -74,6 +74,7 @@ extern const struct efi_device_path_to_text_protocol efi_device_path_to_text;
 
 extern const efi_guid_t efi_guid_console_control;
 extern const efi_guid_t efi_guid_device_path;
+extern const efi_guid_t efi_guid_driver_binding_protocol;
 extern const efi_guid_t efi_guid_loaded_image;
 extern const efi_guid_t efi_guid_device_path_to_text_protocol;
 
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 5a73ea5cd0..1069da7d79 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -18,6 +18,14 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
+static efi_status_t EFIAPI efi_locate_protocol(const efi_guid_t *protocol,
+					       void *registration,
+					       void **protocol_interface);
+static efi_status_t EFIAPI efi_locate_handle_buffer(
+			enum efi_locate_search_type search_type,
+			const efi_guid_t *protocol, void *search_key,
+			unsigned long *no_handles, efi_handle_t **buffer);
+
 /* This list contains all the EFI objects our payload has access to */
 LIST_HEAD(efi_obj_list);
 
@@ -49,6 +57,9 @@ static struct efi_configuration_table __efi_runtime_data efi_conf_table[2];
 static volatile void *efi_gd, *app_gd;
 #endif
 
+const efi_guid_t efi_guid_driver_binding_protocol =
+			EFI_DRIVER_BINDING_PROTOCOL_GUID;
+
 static int entry_count;
 static int nesting_level;
 
@@ -920,15 +931,121 @@ static efi_status_t EFIAPI efi_set_watchdog_timer(unsigned long timeout,
 	return efi_unsupported(__func__);
 }
 
+static efi_status_t efi_bind_controller(
+			efi_handle_t controller_handle,
+			efi_handle_t driver_image_handle,
+			struct efi_device_path *remain_device_path)
+{
+	struct efi_driver_binding_protocol *binding_protocol;
+	efi_status_t r;
+
+	r = EFI_CALL(efi_open_protocol(driver_image_handle,
+				       &efi_guid_driver_binding_protocol,
+				       (void **)&binding_protocol,
+				       driver_image_handle, NULL,
+				       EFI_OPEN_PROTOCOL_GET_PROTOCOL));
+	if (r != EFI_SUCCESS)
+		return r;
+	r = EFI_CALL(binding_protocol->supported(binding_protocol,
+						 controller_handle,
+						 remain_device_path));
+	if (r == EFI_SUCCESS)
+		r = EFI_CALL(binding_protocol->start(binding_protocol,
+						     controller_handle,
+						     remain_device_path));
+	EFI_CALL(efi_close_protocol(driver_image_handle,
+				    &efi_guid_driver_binding_protocol,
+				    driver_image_handle, NULL));
+	return r;
+}
+
+static efi_status_t efi_connect_single_controller(
+			efi_handle_t controller_handle,
+			efi_handle_t *driver_image_handle,
+			struct efi_device_path *remain_device_path)
+{
+	efi_handle_t *buffer;
+	unsigned long count;
+	size_t i;
+	efi_status_t r;
+	size_t connected = 0;
+
+	/* Get buffer with all handles with driver binding protocol */
+	r = EFI_CALL(efi_locate_handle_buffer(by_protocol,
+					      &efi_guid_driver_binding_protocol,
+					      NULL, &count, &buffer));
+	if (r != EFI_SUCCESS)
+		return r;
+
+	/*  Context Override */
+	if (driver_image_handle) {
+		for (; *driver_image_handle; ++driver_image_handle) {
+			for (i = 0; i < count; ++i) {
+				if (buffer[i] == *driver_image_handle) {
+					buffer[i] = NULL;
+					r = efi_bind_controller(
+							controller_handle,
+							*driver_image_handle,
+							remain_device_path);
+					if (r == EFI_SUCCESS)
+						++connected;
+				}
+			}
+		}
+	}
+
+	/*
+	 * Some overrides are not yet implemented:
+	 * Platform Driver Override
+	 * Driver Family Override Search
+	 * Driver Family Override Search
+	 * Bus Specific Driver Override
+	 */
+
+	/* Driver Binding Search */
+	for (i = 0; i < count; ++i) {
+		if (buffer[i]) {
+			r = efi_bind_controller(controller_handle,
+						buffer[i],
+						remain_device_path);
+			if (r == EFI_SUCCESS)
+				++connected;
+		}
+	}
+
+	efi_free_pool(buffer);
+	if (!connected)
+		return EFI_NOT_FOUND;
+	return EFI_SUCCESS;
+}
+
 static efi_status_t EFIAPI efi_connect_controller(
 			efi_handle_t controller_handle,
 			efi_handle_t *driver_image_handle,
 			struct efi_device_path *remain_device_path,
 			bool recursive)
 {
+	efi_status_t r;
+
 	EFI_ENTRY("%p, %p, %p, %d", controller_handle, driver_image_handle,
 		  remain_device_path, recursive);
-	return EFI_EXIT(EFI_NOT_FOUND);
+
+	if (!controller_handle) {
+		r = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+
+	if (recursive) {
+		r = EFI_UNSUPPORTED;
+		goto out;
+	}
+
+	r = efi_connect_single_controller(controller_handle,
+					  driver_image_handle,
+					  remain_device_path);
+
+out:
+	return EFI_EXIT(r);
 }
 
 static efi_status_t EFIAPI efi_disconnect_controller(void *controller_handle,
-- 
2.14.1

[U-Boot] [PATCH 16/23] efi_loader: implement DisconnectController

[Heinrich Schuchardt]

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_boottime.c | 77 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 1069da7d79..c5a17b6252 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1052,9 +1052,84 @@ static efi_status_t EFIAPI efi_disconnect_controller(void *controller_handle,
 						     void *driver_image_handle,
 						     void *child_handle)
 {
+	struct efi_driver_binding_protocol *binding_protocol;
+	efi_handle_t child_handle_buffer;
+	unsigned long driver_count;
+	efi_handle_t *driver_handle_buffer;
+	size_t i;
+	UINTN number_of_children;
+	efi_status_t r;
+	size_t stop_count = 0;
+
 	EFI_ENTRY("%p, %p, %p", controller_handle, driver_image_handle,
 		  child_handle);
-	return EFI_EXIT(EFI_INVALID_PARAMETER);
+
+	if (!efi_search_obj(controller_handle)) {
+		r = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+
+	/* Create list of driver handles */
+	if (driver_image_handle) {
+		driver_handle_buffer = &driver_image_handle,
+		driver_count = 1;
+		/* Check that the handle supports driver binding protocol */
+		r = efi_search_protocol(driver_image_handle,
+					&efi_guid_driver_binding_protocol,
+					NULL);
+	} else {
+		/* Get buffer with all handles with driver binding protocol */
+		r = EFI_CALL(efi_locate_handle_buffer(
+			     by_protocol, &efi_guid_driver_binding_protocol,
+			     NULL, &driver_count, &driver_handle_buffer));
+	}
+	if (r != EFI_SUCCESS)
+		goto out;
+
+	/* Create list of child handles */
+	if (child_handle) {
+		number_of_children = 1;
+		child_handle_buffer = &child_handle;
+	} else {
+		/*
+		 * We do not fully support child handles.
+		 *
+		 * It is unclear from which handle and which protocols the
+		 * list of child controllers should be collected.
+		 */
+		number_of_children = 0;
+		child_handle_buffer = NULL;
+	}
+
+	for (i = 0; i < driver_count; ++i) {
+		r = EFI_CALL(efi_open_protocol(
+			     driver_handle_buffer[i],
+			     &efi_guid_driver_binding_protocol,
+			     (void **)&binding_protocol,
+			     driver_handle_buffer[i], NULL,
+			     EFI_OPEN_PROTOCOL_GET_PROTOCOL));
+		if (r != EFI_SUCCESS)
+			continue;
+
+		r = EFI_CALL(binding_protocol->stop(binding_protocol,
+						    controller_handle,
+						    number_of_children,
+						    child_handle_buffer));
+		if (r == EFI_SUCCESS)
+			++stop_count;
+		EFI_CALL(efi_close_protocol(driver_handle_buffer[i],
+					    &efi_guid_driver_binding_protocol,
+					    driver_handle_buffer[i], NULL));
+	}
+
+	if (driver_image_handle)
+		efi_free_pool(driver_handle_buffer);
+	if (stop_count)
+		r = EFI_SUCCESS;
+	else
+		r = EFI_NOT_FOUND;
+out:
+	return EFI_EXIT(r);
 }
 
 efi_status_t EFIAPI efi_close_protocol(void *handle, const efi_guid_t *protocol,
-- 
2.14.1

[U-Boot] [PATCH 17/23] efi_loader: efi_net: hwaddr_size = 6

[Heinrich Schuchardt]

The length of a MAC address is 6.
We have to set this length in the EFI_SIMPLE_NETWORK_MODE
structure of the EFI_SIMPLE_NETWORK_PROTOCOL.

Without this patch iPXE fails to initialize the network with
error message
SNP MAC(001e0633bcbf,0x0) has invalid hardware address length 0

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_net.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
index 0b949d86e8..75d7974b0e 100644
--- a/lib/efi_loader/efi_net.c
+++ b/lib/efi_loader/efi_net.c
@@ -259,6 +259,7 @@ int efi_net_register(void **handle)
 	netobj->dp_end = dp_end;
 	memcpy(netobj->dp_mac.mac.addr, eth_get_ethaddr(), 6);
 	memcpy(netobj->net_mode.current_address.mac_addr, eth_get_ethaddr(), 6);
+	netobj->net_mode.hwaddr_size = 6;
 	netobj->net_mode.max_packet_size = PKTSIZE;
 
 	netobj->pxe.mode = &netobj->pxe_mode;
-- 
2.14.1

[U-Boot] [PATCH 18/23] efi_net: return EFI_UNSUPPORTED where appropriate

[Heinrich Schuchardt]

U-Boot does not implement all functions of the simple network
protocol. The unimplemented functions return either of
EFI_SUCCESS and EFI_INVALID_PARAMETER.

The UEFI spec foresees to return EFI_UNSUPPORTED in these cases.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_net.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
index 75d7974b0e..38a3a4fac4 100644
--- a/lib/efi_loader/efi_net.c
+++ b/lib/efi_loader/efi_net.c
@@ -81,9 +81,7 @@ static efi_status_t EFIAPI efi_net_receive_filters(
 	EFI_ENTRY("%p, %x, %x, %x, %lx, %p", this, enable, disable,
 		  reset_mcast_filter, mcast_filter_count, mcast_filter);
 
-	/* XXX Do we care? */
-
-	return EFI_EXIT(EFI_SUCCESS);
+	return EFI_EXIT(EFI_UNSUPPORTED);
 }
 
 static efi_status_t EFIAPI efi_net_station_address(
@@ -92,7 +90,7 @@ static efi_status_t EFIAPI efi_net_station_address(
 {
 	EFI_ENTRY("%p, %x, %p", this, reset, new_mac);
 
-	return EFI_EXIT(EFI_INVALID_PARAMETER);
+	return EFI_EXIT(EFI_UNSUPPORTED);
 }
 
 static efi_status_t EFIAPI efi_net_statistics(struct efi_simple_network *this,
@@ -101,7 +99,7 @@ static efi_status_t EFIAPI efi_net_statistics(struct efi_simple_network *this,
 {
 	EFI_ENTRY("%p, %x, %p, %p", this, reset, stat_size, stat_table);
 
-	return EFI_EXIT(EFI_INVALID_PARAMETER);
+	return EFI_EXIT(EFI_UNSUPPORTED);
 }
 
 static efi_status_t EFIAPI efi_net_mcastiptomac(struct efi_simple_network *this,
@@ -121,7 +119,7 @@ static efi_status_t EFIAPI efi_net_nvdata(struct efi_simple_network *this,
 	EFI_ENTRY("%p, %x, %lx, %lx, %p", this, read_write, offset, buffer_size,
 		  buffer);
 
-	return EFI_EXIT(EFI_INVALID_PARAMETER);
+	return EFI_EXIT(EFI_UNSUPPORTED);
 }
 
 static efi_status_t EFIAPI efi_net_get_status(struct efi_simple_network *this,
-- 
2.14.1

[U-Boot] [PATCH 19/23] efi_loader: correct bits of receive_filters bit mask

[Heinrich Schuchardt]

Remove extraneous commas.
Add comment.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_api.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/efi_api.h b/include/efi_api.h
index b2838125d7..029b57ca5e 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -530,11 +530,12 @@ struct efi_simple_network_mode {
 	u8 media_present;
 };
 
-#define EFI_SIMPLE_NETWORK_RECEIVE_UNICAST               0x01,
-#define EFI_SIMPLE_NETWORK_RECEIVE_MULTICAST             0x02,
-#define EFI_SIMPLE_NETWORK_RECEIVE_BROADCAST             0x04,
-#define EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS           0x08,
-#define EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST 0x10,
+/* receive_filters bit mask */
+#define EFI_SIMPLE_NETWORK_RECEIVE_UNICAST               0x01
+#define EFI_SIMPLE_NETWORK_RECEIVE_MULTICAST             0x02
+#define EFI_SIMPLE_NETWORK_RECEIVE_BROADCAST             0x04
+#define EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS           0x08
+#define EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST 0x10
 
 struct efi_simple_network
 {
-- 
2.14.1

[U-Boot] [PATCH 20/23] efi_loader: use events for efi_net_receive

[Heinrich Schuchardt]

A timer event is defined. The timer handler cares for receiving new
packets.

efi_timer_check is called both in efi_net_transmit and efi_net_receive
to enable events during network communication.

Calling efi_timer_check in efi_net_get_status is implemented in a
separate patch.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_net.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
index 38a3a4fac4..7659109386 100644
--- a/lib/efi_loader/efi_net.c
+++ b/lib/efi_loader/efi_net.c
@@ -146,6 +146,8 @@ static efi_status_t EFIAPI efi_net_transmit(struct efi_simple_network *this,
 	EFI_ENTRY("%p, %lx, %lx, %p, %p, %p, %p", this, header_size,
 		  buffer_size, buffer, src_addr, dest_addr, protocol);
 
+	efi_timer_check();
+
 	if (header_size) {
 		/* We would need to create the header if header_size != 0 */
 		return EFI_EXIT(EFI_INVALID_PARAMETER);
@@ -177,9 +179,7 @@ static efi_status_t EFIAPI efi_net_receive(struct efi_simple_network *this,
 	EFI_ENTRY("%p, %p, %p, %p, %p, %p, %p", this, header_size,
 		  buffer_size, buffer, src_addr, dest_addr, protocol);
 
-	push_packet = efi_net_push;
-	eth_rx();
-	push_packet = NULL;
+	efi_timer_check();
 
 	if (!new_rx_packet)
 		return EFI_EXIT(EFI_NOT_READY);
@@ -207,6 +207,21 @@ void efi_net_set_dhcp_ack(void *pkt, int len)
 	memcpy(dhcp_ack, pkt, min(len, maxsize));
 }
 
+static struct efi_event *network_timer_event;
+
+static void EFIAPI efi_network_timer_notify(struct efi_event *event,
+					    void *context)
+{
+	EFI_ENTRY("%p, %p", event, context);
+
+	if (!new_rx_packet) {
+		push_packet = efi_net_push;
+		eth_rx();
+		push_packet = NULL;
+	}
+	EFI_EXIT(EFI_SUCCESS);
+}
+
 /* This gets called from do_bootefi_exec(). */
 int efi_net_register(void **handle)
 {
@@ -221,6 +236,7 @@ int efi_net_register(void **handle)
 		.dp.sub_type = DEVICE_PATH_SUB_TYPE_END,
 		.dp.length = sizeof(dp_end),
 	};
+	efi_status_t r;
 
 	if (!eth_get_dev()) {
 		/* No eth device active, don't expose any */
@@ -270,5 +286,18 @@ int efi_net_register(void **handle)
 	if (handle)
 		*handle = &netobj->net;
 
+	r = efi_create_event(EVT_TIMER | EVT_NOTIFY_SIGNAL, TPL_CALLBACK,
+			     efi_network_timer_notify, NULL,
+			     &network_timer_event);
+	if (r != EFI_SUCCESS) {
+		printf("ERROR: Failed to register network event\n");
+		return r;
+	}
+	/* Network is time critical, call in every timer cyle */
+	r = efi_set_timer(network_timer_event, EFI_TIMER_PERIODIC, 0);
+	if (r != EFI_SUCCESS)
+		printf("ERROR: Failed to set network timer\n");
+	return r;
+
 	return 0;
 }
-- 
2.14.1

[U-Boot] [PATCH 21/23] efi_loader: fix efi_net_get_status

[Heinrich Schuchardt]

The returned interrupt status was wrong.

As out transmit buffer is empty we need to always set
EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT.

When we have received a packet we need to set
EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT.

Furthermore we should call efi_timer_check() to handle events.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_api.h        |  6 ++++++
 lib/efi_loader/efi_net.c | 11 ++++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/include/efi_api.h b/include/efi_api.h
index 029b57ca5e..7d2355c033 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -537,6 +537,12 @@ struct efi_simple_network_mode {
 #define EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS           0x08
 #define EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST 0x10
 
+/* interrupt status bit mask */
+#define EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT	0x01
+#define EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT	0x02
+#define EFI_SIMPLE_NETWORK_COMMAND_INTERRUPT	0x04
+#define EFI_SIMPLE_NETWORK_SOFTWARE_INTERRUPT	0x08
+
 struct efi_simple_network
 {
 	u64 revision;
diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
index 7659109386..e15f403c20 100644
--- a/lib/efi_loader/efi_net.c
+++ b/lib/efi_loader/efi_net.c
@@ -127,9 +127,14 @@ static efi_status_t EFIAPI efi_net_get_status(struct efi_simple_network *this,
 {
 	EFI_ENTRY("%p, %p, %p", this, int_status, txbuf);
 
-	/* We send packets synchronously, so nothing is outstanding */
-	if (int_status)
-		*int_status = 0;
+	efi_timer_check();
+
+	if (int_status) {
+		/* We send packets synchronously, so nothing is outstanding */
+		*int_status = EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
+		if (new_rx_packet)
+			*int_status |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
+	}
 	if (txbuf)
 		*txbuf = new_tx_packet;
 
-- 
2.14.1

[U-Boot] [PATCH 22/23] efi_loader: set parent handle in efi_load_image

[Heinrich Schuchardt]

The parent_handle of the loaded image must be set.
Add the file path protocol from the provided parameter.
Set system table.
Add parameter checks.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_boottime.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index c5a17b6252..477809e4ca 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -774,27 +774,54 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
 	};
 	struct efi_loaded_image *info;
 	struct efi_object *obj;
+	efi_status_t r;
 
 	EFI_ENTRY("%d, %p, %p, %p, %ld, %p", boot_policy, parent_image,
 		  file_path, source_buffer, source_size, image_handle);
+
+	/* We do not support loading by device path, yet. */
+	if (!source_buffer) {
+		r = EFI_NOT_FOUND;
+		goto out;
+	}
+	if (!parent_image || !image_handle) {
+		r = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+
 	info = malloc(sizeof(*info));
+	if (!info) {
+		r = EFI_OUT_OF_RESOURCES;
+		goto out;
+	}
 	loaded_image_info_obj.protocols[0].protocol_interface = info;
+	loaded_image_info_obj.protocols[1].protocol_interface = file_path;
 	obj = malloc(sizeof(loaded_image_info_obj));
+	if (!obj) {
+		free(info);
+		r = EFI_OUT_OF_RESOURCES;
+		goto out;
+	}
 	memset(info, 0, sizeof(*info));
 	memcpy(obj, &loaded_image_info_obj, sizeof(loaded_image_info_obj));
 	obj->handle = info;
+	info->system_table = &systab;
+	info->parent_handle = parent_image;
 	info->file_path = file_path;
 	info->reserved = efi_load_pe(source_buffer, info);
 	if (!info->reserved) {
 		free(info);
 		free(obj);
-		return EFI_EXIT(EFI_UNSUPPORTED);
+		r = EFI_UNSUPPORTED;
+		goto out;
 	}
 
 	*image_handle = info;
 	list_add_tail(&obj->link, &efi_obj_list);
 
-	return EFI_EXIT(EFI_SUCCESS);
+	r = EFI_SUCCESS;
+out:
+	return EFI_EXIT(r);
 }
 
 static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
-- 
2.14.1

[U-Boot] [PATCH 23/23] efi_loader: implement SetWatchdogTimer

[Heinrich Schuchardt]

The watchdog is initialized with a 5 minute timeout period.
It can be reset by SetWatchdogTimer.
It is stopped by ExitBoottimeServices.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 cmd/bootefi.c                 |  1 +
 include/efi_loader.h          |  4 +++
 lib/efi_loader/Makefile       |  2 +-
 lib/efi_loader/efi_boottime.c |  3 ++-
 lib/efi_loader/efi_watchdog.c | 58 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 66 insertions(+), 2 deletions(-)
 create mode 100644 lib/efi_loader/efi_watchdog.c

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 3196d86040..47771f87cc 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -132,6 +132,7 @@ static void efi_init_obj_list(void)
 #ifdef CONFIG_GENERATE_SMBIOS_TABLE
 	efi_smbios_register();
 #endif
+	efi_watchdog_register();
 
 	/* Initialize EFI runtime services */
 	efi_reset_system_init();
diff --git a/include/efi_loader.h b/include/efi_loader.h
index f9f33e1d01..0c1f4e21ca 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -150,11 +150,15 @@ int efi_disk_register(void);
 int efi_gop_register(void);
 /* Called by bootefi to make the network interface available */
 int efi_net_register(void **handle);
+/* 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);
 
 /* Called by networking code to memorize the dhcp ack package */
 void efi_net_set_dhcp_ack(void *pkt, int len);
+/* Called by efi_set_watchdog_timer to reset the timer */
+efi_status_t efi_set_watchdog(unsigned long timeout);
 
 /* Called from places to check whether a timer expired */
 void efi_timer_check(void);
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index 30bf343a36..6bca05aeb4 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -15,7 +15,7 @@ always := $(efiprogs-y)
 
 obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
 obj-y += efi_image_loader.o efi_boottime.o efi_runtime.o efi_console.o
-obj-y += efi_memory.o efi_device_path_to_text.o
+obj-y += efi_memory.o efi_device_path_to_text.o efi_watchdog.o
 obj-$(CONFIG_LCD) += efi_gop.o
 obj-$(CONFIG_DM_VIDEO) += efi_gop.o
 obj-$(CONFIG_PARTITIONS) += efi_disk.o
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 477809e4ca..8f06209794 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -928,6 +928,7 @@ static efi_status_t EFIAPI efi_exit_boot_services(void *image_handle,
 	bootm_disable_interrupts();
 
 	/* Give the payload some time to boot */
+	efi_set_watchdog(0);
 	WATCHDOG_RESET();
 
 	return EFI_EXIT(EFI_SUCCESS);
@@ -955,7 +956,7 @@ static efi_status_t EFIAPI efi_set_watchdog_timer(unsigned long timeout,
 {
 	EFI_ENTRY("%ld, 0x%"PRIx64", %ld, %p", timeout, watchdog_code,
 		  data_size, watchdog_data);
-	return efi_unsupported(__func__);
+	return EFI_EXIT(efi_set_watchdog(timeout));
 }
 
 static efi_status_t efi_bind_controller(
diff --git a/lib/efi_loader/efi_watchdog.c b/lib/efi_loader/efi_watchdog.c
new file mode 100644
index 0000000000..58c098e8bd
--- /dev/null
+++ b/lib/efi_loader/efi_watchdog.c
@@ -0,0 +1,58 @@
+/*
+ *  EFI device path interface
+ *
+ *  Copyright (c) 2017 Heinrich Schuchardt
+ *
+ *  SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#include <common.h>
+#include <efi_loader.h>
+
+static struct efi_event *watchdog_timer_event;
+
+static void EFIAPI efi_watchdog_timer_notify(struct efi_event *event,
+					     void *context)
+{
+	EFI_ENTRY("%p, %p", event, context);
+
+	printf("\nEFI: Watchdog timeout\n");
+	EFI_CALL_VOID(efi_reset_system(EFI_RESET_COLD, EFI_SUCCESS, 0, NULL));
+
+	EFI_EXIT(EFI_UNSUPPORTED);
+}
+
+efi_status_t efi_set_watchdog(unsigned long timeout)
+{
+	efi_status_t r;
+
+	if (timeout)
+		/* Reset watchdog */
+		r = efi_set_timer(watchdog_timer_event, EFI_TIMER_RELATIVE,
+				  10000000 * timeout);
+	else
+		/* Deactivate watchdog */
+		r = efi_set_timer(watchdog_timer_event, EFI_TIMER_STOP, 0);
+	return r;
+}
+
+/* This gets called from do_bootefi_exec(). */
+int efi_watchdog_register(void)
+{
+	efi_status_t r;
+
+	r = efi_create_event(EVT_TIMER | EVT_NOTIFY_SIGNAL, TPL_CALLBACK,
+			     efi_watchdog_timer_notify, NULL,
+			     &watchdog_timer_event);
+	if (r != EFI_SUCCESS) {
+		printf("ERROR: Failed to register watchdog event\n");
+		return r;
+	}
+	/* Set watchdog to trigger after 5 minutes */
+	r = efi_set_watchdog(300);
+	if (r != EFI_SUCCESS) {
+		printf("ERROR: Failed to set watchdog timer\n");
+		return r;
+	}
+	return 0;
+}
-- 
2.14.1

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Simon Glass]

Hi Heinrich,

On 26 August 2017 at 16:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> This patch sequence contains all patches needed to load
> iPXE and use it for downloading and executing images
> via https or http or to mount iSCSI volumes.
>
> Network speed on an Odroid C2 reached 30 MB/s which should be
> enough for most use cases.
>
> I have tested the following iPXE commands successfully
> * dhcp
> * route
> * ntp
> * sanhook iSCSI-target
> * chain http-target
> * kernel http-target
> * boot (after calling kernel)
> * exit
> * reboot
>
> The only adjustment in iPXE was adding file src/config/local/nap.h with
>   #undef NAP_EFIX86
>   #undef NAP_EFIARM
>   #define NAP_NULL
> and src/config/local/myscript.ipxe with
>   #!ipxe
>   shell
> before building iPXE with
>   make bin-arm64-efi/snp.efi EMBED=config/local/myscript.ipxe
>
> The next task will be to put iXPE binaries on a server
> and to create Travis CI test cases.

Some general comments on the series as a whole:

1. It really needs to have tests. This is a lot of new code in U-Boot,
and relying on travis CI test cases (which takes forever to run) is
not a good option. We have a 'make tests' target which you should hook
into, via the pytests framework. This runs in a minute or so. There is
quite a bit of documentation in test/py for this. It should be easy
enough to build up the data structures in sandbox and then test that
each function does what is expected.

2. Exported functions should be commented to describe their purpose,
arguments and return value. Non-trivial static functions should be
commented too.

Regards,
Simon

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Heinrich Schuchardt]

On 08/27/2017 10:10 PM, Simon Glass wrote:
> Hi Heinrich,
> 
> On 26 August 2017 at 16:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> This patch sequence contains all patches needed to load
>> iPXE and use it for downloading and executing images
>> via https or http or to mount iSCSI volumes.
>>
>> Network speed on an Odroid C2 reached 30 MB/s which should be
>> enough for most use cases.
>>
>> I have tested the following iPXE commands successfully
>> * dhcp
>> * route
>> * ntp
>> * sanhook iSCSI-target
>> * chain http-target
>> * kernel http-target
>> * boot (after calling kernel)
>> * exit
>> * reboot
>>
>> The only adjustment in iPXE was adding file src/config/local/nap.h with
>>   #undef NAP_EFIX86
>>   #undef NAP_EFIARM
>>   #define NAP_NULL
>> and src/config/local/myscript.ipxe with
>>   #!ipxe
>>   shell
>> before building iPXE with
>>   make bin-arm64-efi/snp.efi EMBED=config/local/myscript.ipxe
>>
>> The next task will be to put iXPE binaries on a server
>> and to create Travis CI test cases.
> 
> Some general comments on the series as a whole:
> 
> 1. It really needs to have tests. This is a lot of new code in U-Boot,
> and relying on travis CI test cases (which takes forever to run) is
> not a good option. We have a 'make tests' target which you should hook
> into, via the pytests framework. This runs in a minute or so. There is
> quite a bit of documentation in test/py for this. It should be easy
> enough to build up the data structures in sandbox and then test that
> each function does what is expected.
> 
> 2. Exported functions should be commented to describe their purpose,
> arguments and return value. Non-trivial static functions should be
> commented too.
> 
> Regards,
> Simon
> 
Hello Alex,

testing the EFI API is only possible from an EFI program.

I suggest to build an EFI app selftest.efi like the helloworld.efi have.
I would add command
bootefi selftest.efi
to run the tests and provide the python wrapper code to add it to the
test suite.

There are some very simple patches which you may decide to cherry pick,
notably

[PATCH 02/23] efi_loader: notify when ExitBootServices is invoked
[PATCH 03/23] efi_loader: support 16 protocols per efi_object
[PATCH 17/23] efi_loader: efi_net: hwaddr_size = 6
[PATCH 18/23] efi_net: return EFI_UNSUPPORTED where appropriate

Best regards

Heinrich Schuchardt

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Alexander Graf]

On 08/29/2017 12:52 PM, Heinrich Schuchardt wrote:
> On 08/27/2017 10:10 PM, Simon Glass wrote:
>> Hi Heinrich,
>>
>> On 26 August 2017 at 16:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>> This patch sequence contains all patches needed to load
>>> iPXE and use it for downloading and executing images
>>> via https or http or to mount iSCSI volumes.
>>>
>>> Network speed on an Odroid C2 reached 30 MB/s which should be
>>> enough for most use cases.
>>>
>>> I have tested the following iPXE commands successfully
>>> * dhcp
>>> * route
>>> * ntp
>>> * sanhook iSCSI-target
>>> * chain http-target
>>> * kernel http-target
>>> * boot (after calling kernel)
>>> * exit
>>> * reboot
>>>
>>> The only adjustment in iPXE was adding file src/config/local/nap.h with
>>>    #undef NAP_EFIX86
>>>    #undef NAP_EFIARM
>>>    #define NAP_NULL
>>> and src/config/local/myscript.ipxe with
>>>    #!ipxe
>>>    shell
>>> before building iPXE with
>>>    make bin-arm64-efi/snp.efi EMBED=config/local/myscript.ipxe
>>>
>>> The next task will be to put iXPE binaries on a server
>>> and to create Travis CI test cases.
>> Some general comments on the series as a whole:
>>
>> 1. It really needs to have tests. This is a lot of new code in U-Boot,
>> and relying on travis CI test cases (which takes forever to run) is
>> not a good option. We have a 'make tests' target which you should hook
>> into, via the pytests framework. This runs in a minute or so. There is
>> quite a bit of documentation in test/py for this. It should be easy
>> enough to build up the data structures in sandbox and then test that
>> each function does what is expected.
>>
>> 2. Exported functions should be commented to describe their purpose,
>> arguments and return value. Non-trivial static functions should be
>> commented too.
>>
>> Regards,
>> Simon
>>
> Hello Alex,

I think you meant to say Hello Simon?

>
> testing the EFI API is only possible from an EFI program.
>
> I suggest to build an EFI app selftest.efi like the helloworld.efi have.
> I would add command
> bootefi selftest.efi
> to run the tests and provide the python wrapper code to add it to the
> test suite.

I think that's a great idea, yes.

>
> There are some very simple patches which you may decide to cherry pick,
> notably
>
> [PATCH 02/23] efi_loader: notify when ExitBootServices is invoked
> [PATCH 03/23] efi_loader: support 16 protocols per efi_object
> [PATCH 17/23] efi_loader: efi_net: hwaddr_size = 6
> [PATCH 18/23] efi_net: return EFI_UNSUPPORTED where appropriate

How important are those? I would ideally like to see the unit test thing 
first, then add new functionality :)


Alex

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Rob Clark]

On Tue, Aug 29, 2017 at 7:45 AM, Alexander Graf <agraf@suse.de> wrote:
> On 08/29/2017 12:52 PM, Heinrich Schuchardt wrote:
>>
>> On 08/27/2017 10:10 PM, Simon Glass wrote:
>>>
>>> Hi Heinrich,
>>>
>>> On 26 August 2017 at 16:51, Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> wrote:
>>>>
>>>> This patch sequence contains all patches needed to load
>>>> iPXE and use it for downloading and executing images
>>>> via https or http or to mount iSCSI volumes.
>>>>
>>>> Network speed on an Odroid C2 reached 30 MB/s which should be
>>>> enough for most use cases.
>>>>
>>>> I have tested the following iPXE commands successfully
>>>> * dhcp
>>>> * route
>>>> * ntp
>>>> * sanhook iSCSI-target
>>>> * chain http-target
>>>> * kernel http-target
>>>> * boot (after calling kernel)
>>>> * exit
>>>> * reboot
>>>>
>>>> The only adjustment in iPXE was adding file src/config/local/nap.h with
>>>>    #undef NAP_EFIX86
>>>>    #undef NAP_EFIARM
>>>>    #define NAP_NULL
>>>> and src/config/local/myscript.ipxe with
>>>>    #!ipxe
>>>>    shell
>>>> before building iPXE with
>>>>    make bin-arm64-efi/snp.efi EMBED=config/local/myscript.ipxe
>>>>
>>>> The next task will be to put iXPE binaries on a server
>>>> and to create Travis CI test cases.
>>>
>>> Some general comments on the series as a whole:
>>>
>>> 1. It really needs to have tests. This is a lot of new code in U-Boot,
>>> and relying on travis CI test cases (which takes forever to run) is
>>> not a good option. We have a 'make tests' target which you should hook
>>> into, via the pytests framework. This runs in a minute or so. There is
>>> quite a bit of documentation in test/py for this. It should be easy
>>> enough to build up the data structures in sandbox and then test that
>>> each function does what is expected.
>>>
>>> 2. Exported functions should be commented to describe their purpose,
>>> arguments and return value. Non-trivial static functions should be
>>> commented too.
>>>
>>> Regards,
>>> Simon
>>>
>> Hello Alex,
>
>
> I think you meant to say Hello Simon?
>
>>
>> testing the EFI API is only possible from an EFI program.
>>
>> I suggest to build an EFI app selftest.efi like the helloworld.efi have.
>> I would add command
>> bootefi selftest.efi
>> to run the tests and provide the python wrapper code to add it to the
>> test suite.
>
>
> I think that's a great idea, yes.

I wonder how far we are from running UEFI tests (either the official
ones, or I seem to remember hearing about some other test suite which
didn't require UEFI shell)?

That seems more useful long term than re-inventing comprehensive UEFI
test suite.  (Also, beyond just running shim/fallback/grub, I don't
really have time to invent new tests for the stack of efi_loader
patches I have.)

If there is a TODO list of what is required to get to the point where
we can run existing UEFI tests, I can try and pick off bits and pieces
here and there as I have time.

BR,
-R

>>
>> There are some very simple patches which you may decide to cherry pick,
>> notably
>>
>> [PATCH 02/23] efi_loader: notify when ExitBootServices is invoked
>> [PATCH 03/23] efi_loader: support 16 protocols per efi_object
>> [PATCH 17/23] efi_loader: efi_net: hwaddr_size = 6
>> [PATCH 18/23] efi_net: return EFI_UNSUPPORTED where appropriate
>
>
> How important are those? I would ideally like to see the unit test thing
> first, then add new functionality :)
>
>
> Alex
>

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Simon Glass]

Hi Heinrich,

On 29 August 2017 at 18:52, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 08/27/2017 10:10 PM, Simon Glass wrote:
>> Hi Heinrich,
>>
>> On 26 August 2017 at 16:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>> This patch sequence contains all patches needed to load
>>> iPXE and use it for downloading and executing images
>>> via https or http or to mount iSCSI volumes.
>>>
>>> Network speed on an Odroid C2 reached 30 MB/s which should be
>>> enough for most use cases.
>>>
>>> I have tested the following iPXE commands successfully
>>> * dhcp
>>> * route
>>> * ntp
>>> * sanhook iSCSI-target
>>> * chain http-target
>>> * kernel http-target
>>> * boot (after calling kernel)
>>> * exit
>>> * reboot
>>>
>>> The only adjustment in iPXE was adding file src/config/local/nap.h with
>>>   #undef NAP_EFIX86
>>>   #undef NAP_EFIARM
>>>   #define NAP_NULL
>>> and src/config/local/myscript.ipxe with
>>>   #!ipxe
>>>   shell
>>> before building iPXE with
>>>   make bin-arm64-efi/snp.efi EMBED=config/local/myscript.ipxe
>>>
>>> The next task will be to put iXPE binaries on a server
>>> and to create Travis CI test cases.
>>
>> Some general comments on the series as a whole:
>>
>> 1. It really needs to have tests. This is a lot of new code in U-Boot,
>> and relying on travis CI test cases (which takes forever to run) is
>> not a good option. We have a 'make tests' target which you should hook
>> into, via the pytests framework. This runs in a minute or so. There is
>> quite a bit of documentation in test/py for this. It should be easy
>> enough to build up the data structures in sandbox and then test that
>> each function does what is expected.
>>
>> 2. Exported functions should be commented to describe their purpose,
>> arguments and return value. Non-trivial static functions should be
>> commented too.
>>
>> Regards,
>> Simon
>>
> Hello Alex,
>
> testing the EFI API is only possible from an EFI program.

Why is that? Is it not possible to compile the tests into U-Boot
sandbox? It would be much easier to develop that way I think.

>
> I suggest to build an EFI app selftest.efi like the helloworld.efi have.
> I would add command
> bootefi selftest.efi
> to run the tests and provide the python wrapper code to add it to the
> test suite.

That sounds good to me.

>
> There are some very simple patches which you may decide to cherry pick,
> notably
>
> [PATCH 02/23] efi_loader: notify when ExitBootServices is invoked
> [PATCH 03/23] efi_loader: support 16 protocols per efi_object
> [PATCH 17/23] efi_loader: efi_net: hwaddr_size = 6
> [PATCH 18/23] efi_net: return EFI_UNSUPPORTED where appropriate

I will leave that one to Alex.

Regards,
Simon

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Alexander Graf]

On 08/29/2017 02:17 PM, Rob Clark wrote:
> On Tue, Aug 29, 2017 at 7:45 AM, Alexander Graf <agraf@suse.de> wrote:
>> On 08/29/2017 12:52 PM, Heinrich Schuchardt wrote:
>>> On 08/27/2017 10:10 PM, Simon Glass wrote:
>>>> Hi Heinrich,
>>>>
>>>> On 26 August 2017 at 16:51, Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>> wrote:
>>>>> This patch sequence contains all patches needed to load
>>>>> iPXE and use it for downloading and executing images
>>>>> via https or http or to mount iSCSI volumes.
>>>>>
>>>>> Network speed on an Odroid C2 reached 30 MB/s which should be
>>>>> enough for most use cases.
>>>>>
>>>>> I have tested the following iPXE commands successfully
>>>>> * dhcp
>>>>> * route
>>>>> * ntp
>>>>> * sanhook iSCSI-target
>>>>> * chain http-target
>>>>> * kernel http-target
>>>>> * boot (after calling kernel)
>>>>> * exit
>>>>> * reboot
>>>>>
>>>>> The only adjustment in iPXE was adding file src/config/local/nap.h with
>>>>>     #undef NAP_EFIX86
>>>>>     #undef NAP_EFIARM
>>>>>     #define NAP_NULL
>>>>> and src/config/local/myscript.ipxe with
>>>>>     #!ipxe
>>>>>     shell
>>>>> before building iPXE with
>>>>>     make bin-arm64-efi/snp.efi EMBED=config/local/myscript.ipxe
>>>>>
>>>>> The next task will be to put iXPE binaries on a server
>>>>> and to create Travis CI test cases.
>>>> Some general comments on the series as a whole:
>>>>
>>>> 1. It really needs to have tests. This is a lot of new code in U-Boot,
>>>> and relying on travis CI test cases (which takes forever to run) is
>>>> not a good option. We have a 'make tests' target which you should hook
>>>> into, via the pytests framework. This runs in a minute or so. There is
>>>> quite a bit of documentation in test/py for this. It should be easy
>>>> enough to build up the data structures in sandbox and then test that
>>>> each function does what is expected.
>>>>
>>>> 2. Exported functions should be commented to describe their purpose,
>>>> arguments and return value. Non-trivial static functions should be
>>>> commented too.
>>>>
>>>> Regards,
>>>> Simon
>>>>
>>> Hello Alex,
>>
>> I think you meant to say Hello Simon?
>>
>>> testing the EFI API is only possible from an EFI program.
>>>
>>> I suggest to build an EFI app selftest.efi like the helloworld.efi have.
>>> I would add command
>>> bootefi selftest.efi
>>> to run the tests and provide the python wrapper code to add it to the
>>> test suite.
>>
>> I think that's a great idea, yes.
> I wonder how far we are from running UEFI tests (either the official
> ones, or I seem to remember hearing about some other test suite which
> didn't require UEFI shell)?

Let's ask Leif, Ard and Dong.

The official test suite definitely needs the UEFI Shell. Is the suite 
publicly available by now?

> That seems more useful long term than re-inventing comprehensive UEFI
> test suite.  (Also, beyond just running shim/fallback/grub, I don't
> really have time to invent new tests for the stack of efi_loader
> patches I have.)

Yes and no - it depends on the availability of the official suite :/.

>
> If there is a TODO list of what is required to get to the point where
> we can run existing UEFI tests, I can try and pick off bits and pieces
> here and there as I have time.

Let's ask the others :)


Alex

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Leif Lindholm]

On Tue, Aug 29, 2017 at 02:26:48PM +0200, Alexander Graf wrote:
> > > > I would add command
> > > > bootefi selftest.efi
> > > > to run the tests and provide the python wrapper code to add it to the
> > > > test suite.
> > > 
> > > I think that's a great idea, yes.
> > I wonder how far we are from running UEFI tests (either the official
> > ones, or I seem to remember hearing about some other test suite which
> > didn't require UEFI shell)?
> 
> Let's ask Leif, Ard and Dong.
> 
> The official test suite definitely needs the UEFI Shell. Is the suite
> publicly available by now?

In binary form, you can access it already from the links on
http://uefi.org/testtools 

Yes, 2.5 is latest release. No this is not a restriction ... the SCT
releases have been lagging the specification releases a fair bit.

The 2.5a package contains AARCH64, IA32 and X64 support (not ARM).
Not that it couldn't contain ARM, it just hasn't been packaged.

> > That seems more useful long term than re-inventing comprehensive UEFI
> > test suite.  (Also, beyond just running shim/fallback/grub, I don't
> > really have time to invent new tests for the stack of efi_loader
> > patches I have.)
> 
> Yes and no - it depends on the availability of the official suite :/.

UEFI SCT is not yet open source/free software. Obviously, this is
something Linaro has been lobbying for since day one of our
involvement. There used to be little understanding for this, but that
attitude has shifted substantially.

I will let Dong fill in on what the current status actually get the
code into the open is.

/
    Leif

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Rob Clark]

On Tue, Aug 29, 2017 at 8:57 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Aug 29, 2017 at 02:26:48PM +0200, Alexander Graf wrote:
>> > > > I would add command
>> > > > bootefi selftest.efi
>> > > > to run the tests and provide the python wrapper code to add it to the
>> > > > test suite.
>> > >
>> > > I think that's a great idea, yes.
>> > I wonder how far we are from running UEFI tests (either the official
>> > ones, or I seem to remember hearing about some other test suite which
>> > didn't require UEFI shell)?
>>
>> Let's ask Leif, Ard and Dong.
>>
>> The official test suite definitely needs the UEFI Shell. Is the suite
>> publicly available by now?
>
> In binary form, you can access it already from the links on
> http://uefi.org/testtools
>
> Yes, 2.5 is latest release. No this is not a restriction ... the SCT
> releases have been lagging the specification releases a fair bit.
>
> The 2.5a package contains AARCH64, IA32 and X64 support (not ARM).
> Not that it couldn't contain ARM, it just hasn't been packaged.
>
>> > That seems more useful long term than re-inventing comprehensive UEFI
>> > test suite.  (Also, beyond just running shim/fallback/grub, I don't
>> > really have time to invent new tests for the stack of efi_loader
>> > patches I have.)
>>
>> Yes and no - it depends on the availability of the official suite :/.
>
> UEFI SCT is not yet open source/free software. Obviously, this is
> something Linaro has been lobbying for since day one of our
> involvement. There used to be little understanding for this, but that
> attitude has shifted substantially.

So, if/until UEFI SCT is not an option, what about:

  https://01.org/linux-uefi-validation

(thx to pjones for pointing that out to me)

BR,
-R



> I will let Dong fill in on what the current status actually get the
> code into the open is.
>
> /
>     Leif

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Heinrich Schuchardt]

On 08/29/2017 02:57 PM, Leif Lindholm wrote:
> On Tue, Aug 29, 2017 at 02:26:48PM +0200, Alexander Graf wrote:
>>>>> I would add command
>>>>> bootefi selftest.efi
>>>>> to run the tests and provide the python wrapper code to add it to the
>>>>> test suite.
>>>>
>>>> I think that's a great idea, yes.
>>> I wonder how far we are from running UEFI tests (either the official
>>> ones, or I seem to remember hearing about some other test suite which
>>> didn't require UEFI shell)?
>>
>> Let's ask Leif, Ard and Dong.
>>
>> The official test suite definitely needs the UEFI Shell. Is the suite
>> publicly available by now?
> 
> In binary form, you can access it already from the links on
> http://uefi.org/testtools 
> 
> Yes, 2.5 is latest release. No this is not a restriction ... the SCT
> releases have been lagging the specification releases a fair bit.
> 
> The 2.5a package contains AARCH64, IA32 and X64 support (not ARM).
> Not that it couldn't contain ARM, it just hasn't been packaged.
> 
>>> That seems more useful long term than re-inventing comprehensive UEFI
>>> test suite.  (Also, beyond just running shim/fallback/grub, I don't
>>> really have time to invent new tests for the stack of efi_loader
>>> patches I have.)
>>
>> Yes and no - it depends on the availability of the official suite :/.
> 
> UEFI SCT is not yet open source/free software. Obviously, this is
> something Linaro has been lobbying for since day one of our
> involvement. There used to be little understanding for this, but that
> attitude has shifted substantially.
> 
> I will let Dong fill in on what the current status actually get the
> code into the open is.
> 
> /
>     Leif
> 
According to the accompanying information UEFI SCT requires a working
UEFI shell. This rules out this test suite.

Regards

Heinrich

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Leif Lindholm]

On Tue, Aug 29, 2017 at 05:59:41PM +0200, Heinrich Schuchardt wrote:
> On 08/29/2017 02:57 PM, Leif Lindholm wrote:
> > UEFI SCT is not yet open source/free software. Obviously, this is
> > something Linaro has been lobbying for since day one of our
> > involvement. There used to be little understanding for this, but that
> > attitude has shifted substantially.
> > 
> > I will let Dong fill in on what the current status actually get the
> > code into the open is.
> > 
> According to the accompanying information UEFI SCT requires a working
> UEFI shell. This rules out this test suite.

Hey, I was only replying to:

> > On Tue, Aug 29, 2017 at 02:26:48PM +0200, Alexander Graf wrote:
> >> The official test suite definitely needs the UEFI Shell. Is the suite
> >> publicly available by now?

But also, why does requiring a working UEFI Shell rule it out?
I mean, are there any reasons beyond UEFI Shell not currently being
functional under U-Boot?

/
    Leif

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Alexander Graf]

On 08/29/2017 06:06 PM, Leif Lindholm wrote:
> On Tue, Aug 29, 2017 at 05:59:41PM +0200, Heinrich Schuchardt wrote:
>> On 08/29/2017 02:57 PM, Leif Lindholm wrote:
>>> UEFI SCT is not yet open source/free software. Obviously, this is
>>> something Linaro has been lobbying for since day one of our
>>> involvement. There used to be little understanding for this, but that
>>> attitude has shifted substantially.
>>>
>>> I will let Dong fill in on what the current status actually get the
>>> code into the open is.
>>>
>> According to the accompanying information UEFI SCT requires a working
>> UEFI shell. This rules out this test suite.
> Hey, I was only replying to:
>
>>> On Tue, Aug 29, 2017 at 02:26:48PM +0200, Alexander Graf wrote:
>>>> The official test suite definitely needs the UEFI Shell. Is the suite
>>>> publicly available by now?
> But also, why does requiring a working UEFI Shell rule it out?

I don't think it rules it out, it just means it won't easily work today.

> I mean, are there any reasons beyond UEFI Shell not currently being
> functional under U-Boot?

No :). Someone just needs to sit down and debug why it doesn't work step 
by step.


Alex

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Heinrich Schuchardt]

On 08/29/2017 04:16 PM, Rob Clark wrote:
> On Tue, Aug 29, 2017 at 8:57 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> On Tue, Aug 29, 2017 at 02:26:48PM +0200, Alexander Graf wrote:
>>>>>> I would add command
>>>>>> bootefi selftest.efi
>>>>>> to run the tests and provide the python wrapper code to add it to the
>>>>>> test suite.
>>>>>
>>>>> I think that's a great idea, yes.
>>>> I wonder how far we are from running UEFI tests (either the official
>>>> ones, or I seem to remember hearing about some other test suite which
>>>> didn't require UEFI shell)?
>>>
>>> Let's ask Leif, Ard and Dong.
>>>
>>> The official test suite definitely needs the UEFI Shell. Is the suite
>>> publicly available by now?
>>
>> In binary form, you can access it already from the links on
>> http://uefi.org/testtools
>>
>> Yes, 2.5 is latest release. No this is not a restriction ... the SCT
>> releases have been lagging the specification releases a fair bit.
>>
>> The 2.5a package contains AARCH64, IA32 and X64 support (not ARM).
>> Not that it couldn't contain ARM, it just hasn't been packaged.
>>
>>>> That seems more useful long term than re-inventing comprehensive UEFI
>>>> test suite.  (Also, beyond just running shim/fallback/grub, I don't
>>>> really have time to invent new tests for the stack of efi_loader
>>>> patches I have.)
>>>
>>> Yes and no - it depends on the availability of the official suite :/.
>>
>> UEFI SCT is not yet open source/free software. Obviously, this is
>> something Linaro has been lobbying for since day one of our
>> involvement. There used to be little understanding for this, but that
>> attitude has shifted substantially.
> 
> So, if/until UEFI SCT is not an option, what about:
> 
>   https://01.org/linux-uefi-validation
> 
> (thx to pjones for pointing that out to me)
> 
> BR,
> -R



Unfortunately I do not get the image extracted from
https://01.org/linux-uefi-validation/downloads/luv-live-image-23
loaded:

qemu-system-x86_64 -m 1G -bios denx/u-boot.rom -nographic \
-netdev \
user,id=eth0,tftp=tftp,net=192.168.76.0/24,dhcpstart=192.168.76.9 \
-device e1000,netdev=eth0 -machine pc-i440fx-2.8
luv-v2.1_diskboot_mbr_i386_.img

-Boot 2017.09-rc2-P001-00255-g50e25c18b3 (Aug 29 2017 - 00:49:18 +0200)

CPU: x86_64, vendor AMD, device 663h
DRAM:  1 GiB
Using default environment

Video: 640x480x16
Model: QEMU x86 (I440FX)
Net:   e1000: 52:54:00:12:34:56

Warning: e1000#0 using MAC address from ROM
eth0: e1000#0
IDE:   Bus 0: OK Bus 1: OK
  Device 0: Model: QEMU HARDDISK  Firm: 2.5+ Ser#: QM00001
            Type: Hard Disk
            Supports 48-bit addressing
            Capacity: 118.0 MB = 0.1 GB (241664 x 512)
** Can't read Driver Desriptor Block **

Hopefully Petr knows what is wrong in the partition reading logic.

Heinrich

> 
> 
> 
>> I will let Dong fill in on what the current status actually get the
>> code into the open is.
>>
>> /
>>     Leif
> 

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Simon Glass]

Hi,

On 29 August 2017 at 22:16, Rob Clark <robdclark@gmail.com> wrote:
> On Tue, Aug 29, 2017 at 8:57 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> On Tue, Aug 29, 2017 at 02:26:48PM +0200, Alexander Graf wrote:
>>> > > > I would add command
>>> > > > bootefi selftest.efi
>>> > > > to run the tests and provide the python wrapper code to add it to the
>>> > > > test suite.
>>> > >
>>> > > I think that's a great idea, yes.
>>> > I wonder how far we are from running UEFI tests (either the official
>>> > ones, or I seem to remember hearing about some other test suite which
>>> > didn't require UEFI shell)?
>>>
>>> Let's ask Leif, Ard and Dong.
>>>
>>> The official test suite definitely needs the UEFI Shell. Is the suite
>>> publicly available by now?
>>
>> In binary form, you can access it already from the links on
>> http://uefi.org/testtools
>>
>> Yes, 2.5 is latest release. No this is not a restriction ... the SCT
>> releases have been lagging the specification releases a fair bit.
>>
>> The 2.5a package contains AARCH64, IA32 and X64 support (not ARM).
>> Not that it couldn't contain ARM, it just hasn't been packaged.
>>
>>> > That seems more useful long term than re-inventing comprehensive UEFI
>>> > test suite.  (Also, beyond just running shim/fallback/grub, I don't
>>> > really have time to invent new tests for the stack of efi_loader
>>> > patches I have.)
>>>
>>> Yes and no - it depends on the availability of the official suite :/.
>>
>> UEFI SCT is not yet open source/free software. Obviously, this is
>> something Linaro has been lobbying for since day one of our
>> involvement. There used to be little understanding for this, but that
>> attitude has shifted substantially.
>
> So, if/until UEFI SCT is not an option, what about:
>
>   https://01.org/linux-uefi-validation
>
> (thx to pjones for pointing that out to me)

Well in any case I'm not looking for a large functional test suite at
this stage. It certainly could be useful, but not as a replacement for
unit tests. The latter is for fast verification (so that everyone can
run it as part of 'make tests') and easy identification of the
location of bugs.

These new tests should be written in C, run very quickly (similar to
the existing tests) to verify that the code works. Then when each new
feature is added, the test are expanded to cover the new
functionality. Should the code be refactored, the tests should provide
comfort that nothing broke.

>
> BR,
> -R
>
>
>
>> I will let Dong fill in on what the current status actually get the
>> code into the open is.
>>
>> /
>>     Leif

Regards,
Simon

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Alexander Graf]

> Am 29.08.2017 um 22:16 schrieb Simon Glass <sjg@chromium.org>:
> 
> Hi,
> 
>> On 29 August 2017 at 22:16, Rob Clark <robdclark@gmail.com> wrote:
>>> On Tue, Aug 29, 2017 at 8:57 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>> On Tue, Aug 29, 2017 at 02:26:48PM +0200, Alexander Graf wrote:
>>>>>>> I would add command
>>>>>>> bootefi selftest.efi
>>>>>>> to run the tests and provide the python wrapper code to add it to the
>>>>>>> test suite.
>>>>>> 
>>>>>> I think that's a great idea, yes.
>>>>> I wonder how far we are from running UEFI tests (either the official
>>>>> ones, or I seem to remember hearing about some other test suite which
>>>>> didn't require UEFI shell)?
>>>> 
>>>> Let's ask Leif, Ard and Dong.
>>>> 
>>>> The official test suite definitely needs the UEFI Shell. Is the suite
>>>> publicly available by now?
>>> 
>>> In binary form, you can access it already from the links on
>>> http://uefi.org/testtools
>>> 
>>> Yes, 2.5 is latest release. No this is not a restriction ... the SCT
>>> releases have been lagging the specification releases a fair bit.
>>> 
>>> The 2.5a package contains AARCH64, IA32 and X64 support (not ARM).
>>> Not that it couldn't contain ARM, it just hasn't been packaged.
>>> 
>>>>> That seems more useful long term than re-inventing comprehensive UEFI
>>>>> test suite.  (Also, beyond just running shim/fallback/grub, I don't
>>>>> really have time to invent new tests for the stack of efi_loader
>>>>> patches I have.)
>>>> 
>>>> Yes and no - it depends on the availability of the official suite :/.
>>> 
>>> UEFI SCT is not yet open source/free software. Obviously, this is
>>> something Linaro has been lobbying for since day one of our
>>> involvement. There used to be little understanding for this, but that
>>> attitude has shifted substantially.
>> 
>> So, if/until UEFI SCT is not an option, what about:
>> 
>>  https://01.org/linux-uefi-validation
>> 
>> (thx to pjones for pointing that out to me)
> 
> Well in any case I'm not looking for a large functional test suite at
> this stage. It certainly could be useful, but not as a replacement for
> unit tests. The latter is for fast verification (so that everyone can
> run it as part of 'make tests') and easy identification of the
> location of bugs.

I fail to see the point here to be honest. The thing we care most about in the uefi implementation is abi compatibility with the spec. That's basically what SCT is supposed to get you. Running it should be a matter of seconds with kvm enabled, so 'make tests' should easily be able to run it if you have the correct target compiled.

> 
> These new tests should be written in C, run very quickly (similar to
> the existing tests) to verify that the code works. Then when each new
> feature is added, the test are expanded to cover the new
> functionality. Should the code be refactored, the tests should provide
> comfort that nothing broke.

So far I've never seen that approach work well in (low level) open source projects, as you start to heavily raise the barrier for participation. Most people in this world do these tasks in their spare time and won't spend x2 the time to also write and update unit tests.

I really think having a working functional test suite that is easy to run is the best way forward in this case. We can then start thinking of ways to test hard to reach code sections to increase test coverage.


Alex

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Heinrich Schuchardt]

On 08/29/2017 10:38 PM, Alexander Graf wrote:
> 
> 
>> Am 29.08.2017 um 22:16 schrieb Simon Glass <sjg@chromium.org>:
>>
>> Hi,
>>
>>> On 29 August 2017 at 22:16, Rob Clark <robdclark@gmail.com> wrote:
>>>> On Tue, Aug 29, 2017 at 8:57 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>>> On Tue, Aug 29, 2017 at 02:26:48PM +0200, Alexander Graf wrote:
>>>>>>>> I would add command
>>>>>>>> bootefi selftest.efi
>>>>>>>> to run the tests and provide the python wrapper code to add it to the
>>>>>>>> test suite.
>>>>>>>
>>>>>>> I think that's a great idea, yes.
>>>>>> I wonder how far we are from running UEFI tests (either the official
>>>>>> ones, or I seem to remember hearing about some other test suite which
>>>>>> didn't require UEFI shell)?
>>>>>
>>>>> Let's ask Leif, Ard and Dong.
>>>>>
>>>>> The official test suite definitely needs the UEFI Shell. Is the suite
>>>>> publicly available by now?
>>>>
>>>> In binary form, you can access it already from the links on
>>>> http://uefi.org/testtools
>>>>
>>>> Yes, 2.5 is latest release. No this is not a restriction ... the SCT
>>>> releases have been lagging the specification releases a fair bit.
>>>>
>>>> The 2.5a package contains AARCH64, IA32 and X64 support (not ARM).
>>>> Not that it couldn't contain ARM, it just hasn't been packaged.
>>>>
>>>>>> That seems more useful long term than re-inventing comprehensive UEFI
>>>>>> test suite.  (Also, beyond just running shim/fallback/grub, I don't
>>>>>> really have time to invent new tests for the stack of efi_loader
>>>>>> patches I have.)
>>>>>
>>>>> Yes and no - it depends on the availability of the official suite :/.
>>>>
>>>> UEFI SCT is not yet open source/free software. Obviously, this is
>>>> something Linaro has been lobbying for since day one of our
>>>> involvement. There used to be little understanding for this, but that
>>>> attitude has shifted substantially.
>>>
>>> So, if/until UEFI SCT is not an option, what about:
>>>
>>>  https://01.org/linux-uefi-validation
>>>
>>> (thx to pjones for pointing that out to me)
>>
>> Well in any case I'm not looking for a large functional test suite at
>> this stage. It certainly could be useful, but not as a replacement for
>> unit tests. The latter is for fast verification (so that everyone can
>> run it as part of 'make tests') and easy identification of the
>> location of bugs.
> 
> I fail to see the point here to be honest. The thing we care most about in the uefi implementation is abi compatibility with the spec. That's basically what SCT is supposed to get you. Running it should be a matter of seconds with kvm enabled, so 'make tests' should easily be able to run it if you have the correct target compiled.
> 
>>
>> These new tests should be written in C, run very quickly (similar to
>> the existing tests) to verify that the code works. Then when each new
>> feature is added, the test are expanded to cover the new
>> functionality. Should the code be refactored, the tests should provide
>> comfort that nothing broke.
> 
> So far I've never seen that approach work well in (low level) open source projects, as you start to heavily raise the barrier for participation. Most people in this world do these tasks in their spare time and won't spend x2 the time to also write and update unit tests.
> 
> I really think having a working functional test suite that is easy to run is the best way forward in this case. We can then start thinking of ways to test hard to reach code sections to increase test coverage.
> 
> 
> Alex

Simon is right in that test code is needed to ensure that refactoring
does not break existing functionality. The current git HEAD cannot read
from an IDE disk anymore. This problem could have been caught by a
functional test four weeks ago.

Unit tests should be based on exposed APIs. Tests for lower level
functions would not survive refactoring.

We implement only part of the UEFI spec. If we do not want to blow up
binary size we may even want to keep it that way. So a test suite for
full UEFI compatibility will always fail partially if it runs at all.

I am able to provide a test application that will cover the API
functions that I have focused on (events, protocols, (dis)connect
controllers). To limit my effort I would like to write it for the HEAD
of my patch queue and not break it down into one test patch per
implementation patch. I hope that is ok for you. My effort estimate is
40h+ so, please, be patient.

I would be happy to receive some review for the queued patches in the
meanwhile.

Best regards

Heinrich

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Leif Lindholm]

On Wed, Aug 30, 2017 at 12:03:16AM +0200, Heinrich Schuchardt wrote:
> On 08/29/2017 10:38 PM, Alexander Graf wrote:
> >> Am 29.08.2017 um 22:16 schrieb Simon Glass <sjg@chromium.org>:
> >>>>>> That seems more useful long term than re-inventing comprehensive UEFI
> >>>>>> test suite.  (Also, beyond just running shim/fallback/grub, I don't
> >>>>>> really have time to invent new tests for the stack of efi_loader
> >>>>>> patches I have.)
> >>>>>
> >>>>> Yes and no - it depends on the availability of the official suite :/.
> >>>>
> >>>> UEFI SCT is not yet open source/free software. Obviously, this is
> >>>> something Linaro has been lobbying for since day one of our
> >>>> involvement. There used to be little understanding for this, but that
> >>>> attitude has shifted substantially.
> >>>
> >>> So, if/until UEFI SCT is not an option, what about:
> >>>
> >>>  https://01.org/linux-uefi-validation
> >>>
> >>> (thx to pjones for pointing that out to me)
> >>
> >> Well in any case I'm not looking for a large functional test suite at
> >> this stage. It certainly could be useful, but not as a replacement for
> >> unit tests. The latter is for fast verification (so that everyone can
> >> run it as part of 'make tests') and easy identification of the
> >> location of bugs.
> > 
> > I fail to see the point here to be honest. The thing we care most
> > about in the uefi implementation is abi compatibility with the
> > spec. That's basically what SCT is supposed to get you. Running it
> > should be a matter of seconds with kvm enabled, so 'make tests'
> > should easily be able to run it if you have the correct target
> > compiled.
> > 
> >>
> >> These new tests should be written in C, run very quickly (similar to
> >> the existing tests) to verify that the code works. Then when each new
> >> feature is added, the test are expanded to cover the new
> >> functionality. Should the code be refactored, the tests should provide
> >> comfort that nothing broke.
> > 
> > So far I've never seen that approach work well in (low level) open
> > source projects, as you start to heavily raise the barrier for
> > participation. Most people in this world do these tasks in their
> > spare time and won't spend x2 the time to also write and update
> > unit tests.
> > 
> > I really think having a working functional test suite that is easy
> > to run is the best way forward in this case. We can then start
> > thinking of ways to test hard to reach code sections to increase
> > test coverage.
> > 
> > Alex
> 
> Simon is right in that test code is needed to ensure that refactoring
> does not break existing functionality. The current git HEAD cannot read
> from an IDE disk anymore. This problem could have been caught by a
> functional test four weeks ago.
> 
> Unit tests should be based on exposed APIs. Tests for lower level
> functions would not survive refactoring.

Completely agreed. UEFI is an interface, not an implementation.

> We implement only part of the UEFI spec. If we do not want to blow up
> binary size we may even want to keep it that way. So a test suite for
> full UEFI compatibility will always fail partially if it runs at all.

SCT is (almost?) entirely implemented in the form of:
- Does the implementation claim to support X?
- If so, determine if X behaves in conformance with the specification.

> I am able to provide a test application that will cover the API
> functions that I have focused on (events, protocols, (dis)connect
> controllers). To limit my effort I would like to write it for the HEAD
> of my patch queue and not break it down into one test patch per
> implementation patch. I hope that is ok for you. My effort estimate is
> 40h+ so, please, be patient.

I am not going to claim that getting SCT to run under U-Boot is going
to come near the top of my priority queue any time soon, and it would
certainly be useful for some sort of "make sure we don't break what we
have" tests.

But there will be additional bits of UEFI functionality that will be
useful to add. Some of that functionality will be generically useful,
some may not be relevant for certain systems and if the effect on code
size is non-negligable, its inclusion in the final image should be
made conditional.

And surely it would be preferable to be able to reuse an existing
testsuite when adding new bits, instead of having to implement new
tests for every feature? Especially when the goal is compatibility
with something that uses that existing test suite.

Alex got me up and running with a test environment, so I will put
"investigate why Shell.efi won't run" on my background task list.

/
    Leif

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Simon Glass]

Hi Heinrich,

On 30 August 2017 at 06:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
> On 08/29/2017 10:38 PM, Alexander Graf wrote:
>>
>>
>>> Am 29.08.2017 um 22:16 schrieb Simon Glass <sjg@chromium.org>:
>>>
>>> Hi,
>>>
>>>> On 29 August 2017 at 22:16, Rob Clark <robdclark@gmail.com> wrote:
>>>>> On Tue, Aug 29, 2017 at 8:57 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>>>> On Tue, Aug 29, 2017 at 02:26:48PM +0200, Alexander Graf wrote:
>>>>>>>>> I would add command
>>>>>>>>> bootefi selftest.efi
>>>>>>>>> to run the tests and provide the python wrapper code to add it to the
>>>>>>>>> test suite.
>>>>>>>>
>>>>>>>> I think that's a great idea, yes.
>>>>>>> I wonder how far we are from running UEFI tests (either the official
>>>>>>> ones, or I seem to remember hearing about some other test suite which
>>>>>>> didn't require UEFI shell)?
>>>>>>
>>>>>> Let's ask Leif, Ard and Dong.
>>>>>>
>>>>>> The official test suite definitely needs the UEFI Shell. Is the suite
>>>>>> publicly available by now?
>>>>>
>>>>> In binary form, you can access it already from the links on
>>>>> http://uefi.org/testtools
>>>>>
>>>>> Yes, 2.5 is latest release. No this is not a restriction ... the SCT
>>>>> releases have been lagging the specification releases a fair bit.
>>>>>
>>>>> The 2.5a package contains AARCH64, IA32 and X64 support (not ARM).
>>>>> Not that it couldn't contain ARM, it just hasn't been packaged.
>>>>>
>>>>>>> That seems more useful long term than re-inventing comprehensive UEFI
>>>>>>> test suite.  (Also, beyond just running shim/fallback/grub, I don't
>>>>>>> really have time to invent new tests for the stack of efi_loader
>>>>>>> patches I have.)
>>>>>>
>>>>>> Yes and no - it depends on the availability of the official suite :/.
>>>>>
>>>>> UEFI SCT is not yet open source/free software. Obviously, this is
>>>>> something Linaro has been lobbying for since day one of our
>>>>> involvement. There used to be little understanding for this, but that
>>>>> attitude has shifted substantially.
>>>>
>>>> So, if/until UEFI SCT is not an option, what about:
>>>>
>>>>  https://01.org/linux-uefi-validation
>>>>
>>>> (thx to pjones for pointing that out to me)
>>>
>>> Well in any case I'm not looking for a large functional test suite at
>>> this stage. It certainly could be useful, but not as a replacement for
>>> unit tests. The latter is for fast verification (so that everyone can
>>> run it as part of 'make tests') and easy identification of the
>>> location of bugs.
>>
>> I fail to see the point here to be honest. The thing we care most about in the uefi implementation is abi compatibility with the spec. That's basically what SCT is supposed to get you. Running it should be a matter of seconds with kvm enabled, so 'make tests' should easily be able to run it if you have the correct target compiled.
>>
>>>
>>> These new tests should be written in C, run very quickly (similar to
>>> the existing tests) to verify that the code works. Then when each new
>>> feature is added, the test are expanded to cover the new
>>> functionality. Should the code be refactored, the tests should provide
>>> comfort that nothing broke.
>>
>> So far I've never seen that approach work well in (low level) open source projects, as you start to heavily raise the barrier for participation. Most people in this world do these tasks in their spare time and won't spend x2 the time to also write and update unit tests.

To Alex:

We need to encourage people to write tests in U-Boot for core code. As
a project we spend a lot more time maintaining and refactoring core
code than we do writing it. Also if people write the tests as they
develop it they generally discover some benefits - e.g. it is faster
to get something working if you have a quick test for it. There is
absolutely no way I could have developed driver model in any
reasonable time without tests.

U-Boot now has a fairly rich set of tests. There still are many gaps,
but this gain has been hard won and it is very valuable to the project
and future developers and users of U-Boot.

The problem with large functional tests is that they are too slow and
painful (and often flaky) for people to use in their normal
development flow. It takes a minute to run 'make tests' before sending
out a patch series, and you can particular tests in seconds. This is
very helpful for people to see that they have not broken something.
Breakages in core code are relatively expensive to debug, surprising
to users and adversely affect the reputation of the project.

>>
>> I really think having a working functional test suite that is easy to run is the best way forward in this case. We can then start thinking of ways to test hard to reach code sections to increase test coverage.
>>
>>
>> Alex
>
> Simon is right in that test code is needed to ensure that refactoring
> does not break existing functionality. The current git HEAD cannot read
> from an IDE disk anymore. This problem could have been caught by a
> functional test four weeks ago.
>
> Unit tests should be based on exposed APIs. Tests for lower level
> functions would not survive refactoring.
>
> We implement only part of the UEFI spec. If we do not want to blow up
> binary size we may even want to keep it that way. So a test suite for
> full UEFI compatibility will always fail partially if it runs at all.
>
> I am able to provide a test application that will cover the API
> functions that I have focused on (events, protocols, (dis)connect
> controllers). To limit my effort I would like to write it for the HEAD
> of my patch queue and not break it down into one test patch per
> implementation patch. I hope that is ok for you. My effort estimate is
> 40h+ so, please, be patient.

Yes that sounds like the right approach. I'd like to check about the
point you made about needing to write an EFI app to test things. Is it
not possible to build the test into the same binary as sandbox, and
make internal calls to the EFI API functions?

>
> I would be happy to receive some review for the queued patches in the
> meanwhile.

It is in my queue :-)

Regards,
Simon

[U-Boot] [PATCH 02/23] efi_loader: notify when ExitBootServices is invoked

[Simon Glass]

On 27 August 2017 at 06:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> All events of type EVT_SIGNAL_EXIT_BOOT_SERVICES have to be
> notified when ExitBootServices is invoked.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_boottime.c | 9 +++++++++
>  1 file changed, 9 insertions(+)

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

(I'm reviewing these in advance of the tests)

[U-Boot] [PATCH 03/23] efi_loader: support 16 protocols per efi_object

[Simon Glass]

On 27 August 2017 at 06:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> 8 protocols per efi_object is insufficient for iPXE.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/efi_loader.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

[U-Boot] [PATCH 01/23] efi_loader: allow return value in EFI_CALL

[Simon Glass]

On 27 August 2017 at 06:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Macro EFI_CALL was introduced to call an UEFI function.

Should this be 'an EFI'. Or 'a UEFI'?

> Unfortunately it does not support return values.
> Most UEFI functions have a return value.
>
> So let's rename EFI_CALL to EFI_CALL_VOID and introduce a
> new EFI_CALL macro that supports return values.
>
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/efi_loader.h          | 17 +++++++++++++++--
>  lib/efi_loader/efi_boottime.c |  3 ++-
>  2 files changed, 17 insertions(+), 3 deletions(-)

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

[U-Boot] [PATCH 04/23] efi_loader: rework efi_locate_handle

[Simon Glass]

Hi Heinrich,

On 27 August 2017 at 06:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Check the parameters in efi_locate_handle.
>
> Use list_for_each_entry instead of list_for_each.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_boottime.c | 42 +++++++++++++++++++++++++++++++-----------
>  1 file changed, 31 insertions(+), 11 deletions(-)
>

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

nits below

> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index b5538e0769..570a5ea186 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -599,6 +599,7 @@ static int efi_search(enum efi_locate_search_type search_type,
>         case all_handles:
>                 return 0;
>         case by_register_notify:
> +               /* RegisterProtocolNotify is not implemented yet */
>                 return -1;
>         case by_protocol:
>                 for (i = 0; i < ARRAY_SIZE(efiobj->protocols); i++) {
> @@ -617,16 +618,38 @@ static efi_status_t efi_locate_handle(
>                         efi_guid_t *protocol, void *search_key,
>                         unsigned long *buffer_size, efi_handle_t *buffer)

function needs a comment

>  {
> -       struct list_head *lhandle;
> +       struct efi_object *efiobj;
>         unsigned long size = 0;
>
> +       /* Check parameters */
> +       switch (search_type) {
> +       case all_handles:
> +               break;
> +       case by_register_notify:
> +               if (!search_key)
> +                       return EFI_INVALID_PARAMETER;
> +               /* RegisterProtocolNotify is not implemented yet */
> +               return EFI_UNSUPPORTED;
> +       case by_protocol:
> +               if (!protocol)
> +                       return EFI_INVALID_PARAMETER;
> +               break;
> +       default:
> +               return EFI_INVALID_PARAMETER;
> +       }
> +
> +       /*
> +        * efi_locate_handle_buffer uses this function for
> +        * the calculation of the necessary buffer size.
> +        * So do not require a buffer for buffersize == 0.
> +        */
> +       if (!buffer_size || (*buffer_size && !buffer))
> +               return EFI_INVALID_PARAMETER;
> +
>         /* Count how much space we need */
> -       list_for_each(lhandle, &efi_obj_list) {
> -               struct efi_object *efiobj;
> -               efiobj = list_entry(lhandle, struct efi_object, link);
> -               if (!efi_search(search_type, protocol, search_key, efiobj)) {
> +       list_for_each_entry(efiobj, &efi_obj_list, link) {
> +               if (!efi_search(search_type, protocol, search_key, efiobj))
>                         size += sizeof(void*);
> -               }
>         }
>
>         if (*buffer_size < size) {
> @@ -639,12 +662,9 @@ static efi_status_t efi_locate_handle(
>                 return EFI_NOT_FOUND;
>
>         /* Then fill the array */
> -       list_for_each(lhandle, &efi_obj_list) {
> -               struct efi_object *efiobj;
> -               efiobj = list_entry(lhandle, struct efi_object, link);
> -               if (!efi_search(search_type, protocol, search_key, efiobj)) {
> +       list_for_each_entry(efiobj, &efi_obj_list, link) {
> +               if (!efi_search(search_type, protocol, search_key, efiobj))
>                         *(buffer++) = efiobj->handle;

*buffer++

> -               }
>         }
>
>         return EFI_SUCCESS;
> --
> 2.14.1
>

Regards,
Simon

[U-Boot] [PATCH 05/23] efi_loader: rework efi_search_obj

[Simon Glass]

On 27 August 2017 at 06:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> EFI_HANDLEs are used both in boottime and in runtime services.
> efi_search_obj is a function that can be used to validate
> handles. So let's make it accessible via efi_loader.h.
>
> We can simplify the coding using list_for_each_entry.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/efi_loader.h          | 2 ++
>  lib/efi_loader/efi_boottime.c | 8 +++-----
>  2 files changed, 5 insertions(+), 5 deletions(-)

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

[U-Boot] [PATCH 06/23] efi_loader: new function efi_search_protocol

[Simon Glass]

On 27 August 2017 at 06:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> In multiple functions we are searching for the protocol of a handle.
> This patch provides a new function efi_search_protocol that we can
> use to avoid duplicating code.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_boottime.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)

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

>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index b643d299b9..9dae02daca 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -453,6 +453,31 @@ static efi_status_t EFIAPI efi_check_event(struct efi_event *event)
>         return EFI_EXIT(EFI_INVALID_PARAMETER);
>  }
>
> +static efi_status_t efi_search_protocol(void *handle, efi_guid_t *protocol_guid,
> +                                       struct efi_handler **handler)

Needs a function comment

> +{
> +       struct efi_object *efiobj;
> +       size_t i;
> +       struct efi_handler *protocol;
> +
> +       if (!handle || !protocol_guid)
> +               return EFI_INVALID_PARAMETER;
> +       efiobj = efi_search_obj(handle);
> +       if (!efiobj)
> +               return EFI_INVALID_PARAMETER;
> +       for (i = 0; i < ARRAY_SIZE(efiobj->protocols); i++) {
> +               protocol = &efiobj->protocols[i];
> +               if (!protocol->guid)
> +                       continue;
> +               if (!guidcmp(protocol->guid, protocol_guid)) {
> +                       if (handler)
> +                               *handler = protocol;
> +                       return EFI_SUCCESS;
> +               }
> +       }
> +       return EFI_NOT_FOUND;
> +}
> +
>  static efi_status_t EFIAPI efi_install_protocol_interface(void **handle,
>                         efi_guid_t *protocol, int protocol_interface_type,
>                         void *protocol_interface)
> --
> 2.14.1
>

[U-Boot] [PATCH 07/23] efi_loader: simplify efi_install_protocol_interface

[Simon Glass]

On 27 August 2017 at 06:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Use function efi_search_obj and efi_search_protocol
> to simplify the coding.
>
> Do away with efi_install_protocol_interface_ext.
> We can use EFI_CALL for internal usage.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_boottime.c | 76 ++++++++++++++++++-------------------------
>  1 file changed, 31 insertions(+), 45 deletions(-)
>

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

[U-Boot] [PATCH 08/23] efi_loader: allow creating new handles

[Simon Glass]

On 27 August 2017 at 06:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> In efi_install_protocol_interface support creating
> a new handle.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_boottime.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
Reviewed-by: Simon Glass <sjg@chromium.org>

The use of void ** seems odd. Is that mandated by EFI? Is there no way
to use a proper type?

> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 96cb1fa410..9f8d64659f 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -238,6 +238,23 @@ static efi_status_t EFIAPI efi_free_pool_ext(void *buffer)
>         return EFI_EXIT(r);
>  }
>
> +static efi_status_t efi_create_handle(void **handle)
> +{
> +       struct efi_object *obj;
> +       efi_status_t r;
> +
> +       r = efi_allocate_pool(EFI_ALLOCATE_ANY_PAGES,
> +                             sizeof(struct efi_object),
> +                             (void **)&obj);
> +       if (r != EFI_SUCCESS)
> +               return r;
> +       memset(obj, 0, sizeof(struct efi_object));
> +       obj->handle = obj;
> +       list_add_tail(&obj->link, &efi_obj_list);
> +       *handle = obj;
> +       return r;
> +}
> +
>  /*
>   * Our event capabilities are very limited. Only a small limited
>   * number of events is allowed to coexist.
> @@ -497,8 +514,9 @@ static efi_status_t EFIAPI efi_install_protocol_interface(void **handle,
>
>         /* Create new handle if requested. */
>         if (!*handle) {
> -               r = EFI_OUT_OF_RESOURCES;
> -               goto out;
> +               r = efi_create_handle(handle);
> +               if (r != EFI_SUCCESS)
> +                       goto out;
>         }
>
>         /* Find object. */
> --
> 2.14.1
>

[U-Boot] [PATCH 09/23] efi_loader: simplify efi_uninstall_protocol_interface

[Simon Glass]

On 27 August 2017 at 06:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Use function efi_search_obj and efi_search_protocol
> to simplify the coding.
>
> Do away with efi_uninstall_protocol_interface_ext.
> We can use EFI_CALL for internal usage.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_boottime.c | 56 ++++++++++++++-----------------------------
>  1 file changed, 18 insertions(+), 38 deletions(-)

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

[U-Boot] [PATCH 10/23] efi_loader: open_info in OpenProtocol

[Simon Glass]

Hi Heinrich,

On 27 August 2017 at 06:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> efi_open_protocol and close_protocol have to keep track of
> opened protocols.
>
> So we add an array open_info to each protocol of each handle.
>
> OpenProtocol has enter the agent and controller handle
> information into this array.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/efi_loader.h          |   1 +
>  lib/efi_loader/efi_boottime.c | 130 +++++++++++++++++++++++++++++++-----------
>  2 files changed, 99 insertions(+), 32 deletions(-)
>

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

Please see below.

> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 193fca24ce..2c3360534b 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -87,6 +87,7 @@ extern unsigned int __efi_runtime_rel_start, __efi_runtime_rel_stop;
>  struct efi_handler {
>         const efi_guid_t *guid;
>         void *protocol_interface;
> +       struct efi_open_protocol_info_entry open_info[4];
>  };
>
>  /*
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index a483b827cd..294bc1f138 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1152,24 +1152,111 @@ static void EFIAPI efi_set_mem(void *buffer, unsigned long size, uint8_t value)
>         memset(buffer, value, size);
>  }
>
> +static efi_status_t efi_protocol_open(
> +                       struct efi_handler *protocol,
> +                       void **protocol_interface, void *agent_handle,
> +                       void *controller_handle, uint32_t attributes)
> +{
> +       bool opened_exclusive = false;
> +       bool opened_by_driver = false;
> +       int i;
> +       struct efi_open_protocol_info_entry *open_info;
> +       struct efi_open_protocol_info_entry *match = NULL;
> +
> +       if (attributes !=
> +           EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
> +               *protocol_interface = NULL;
> +       }
> +
> +       for (i = 0; i < ARRAY_SIZE(protocol->open_info); ++i) {
> +               open_info = &protocol->open_info[i];
> +
> +               if (!open_info->open_count)
> +                       continue;
> +               if (open_info->agent_handle == agent_handle) {
> +                       if ((attributes & EFI_OPEN_PROTOCOL_BY_DRIVER) &&
> +                           (open_info->attributes == attributes))
> +                               return EFI_ALREADY_STARTED;
> +                       if (open_info->controller_handle == controller_handle)
> +                               match = open_info;
> +               }
> +               if (open_info->attributes & EFI_OPEN_PROTOCOL_EXCLUSIVE)
> +                       opened_exclusive = true;
> +       }
> +
> +       if (attributes &
> +           (EFI_OPEN_PROTOCOL_EXCLUSIVE | EFI_OPEN_PROTOCOL_BY_DRIVER) &&
> +           opened_exclusive)
> +               return EFI_ACCESS_DENIED;
> +
> +       if (attributes & EFI_OPEN_PROTOCOL_EXCLUSIVE) {
> +               for (i = 0; i < ARRAY_SIZE(protocol->open_info); ++i) {

Is it possible to put this search in a utility function with suitable
parameters and call it? This function is very long.

> +                       open_info = &protocol->open_info[i];
> +
> +                       if (!open_info->open_count)
> +                               continue;
> +                       if (open_info->attributes ==
> +                                       EFI_OPEN_PROTOCOL_BY_DRIVER)
> +                               EFI_CALL(efi_disconnect_controller(
> +                                               open_info->controller_handle,
> +                                               open_info->agent_handle,
> +                                               NULL));
> +               }
> +               opened_by_driver = false;
> +               for (i = 0; i < ARRAY_SIZE(protocol->open_info); ++i) {
> +                       open_info = &protocol->open_info[i];
> +
> +                       if (!open_info->open_count)
> +                               continue;
> +                       if (open_info->attributes & EFI_OPEN_PROTOCOL_BY_DRIVER)
> +                               opened_by_driver = true;
> +               }
> +               if (opened_by_driver)
> +                       return EFI_ACCESS_DENIED;
> +               if (match && !match->open_count)
> +                       match = NULL;
> +       }
> +
> +       /*
> +        * Find an empty slot.
> +        */
> +       if (!match) {
> +               for (i = 0; i < ARRAY_SIZE(protocol->open_info); ++i) {
> +                       open_info = &protocol->open_info[i];
> +
> +                       if (!open_info->open_count) {
> +                               match = open_info;
> +                               break;
> +                       }
> +               }
> +       }
> +       if (!match)
> +               return EFI_OUT_OF_RESOURCES;
> +
> +       match->agent_handle = agent_handle;
> +       match->controller_handle = controller_handle;
> +       match->attributes = attributes;
> +       match->open_count++;
> +       *protocol_interface = protocol->protocol_interface;
> +
> +       return EFI_SUCCESS;
> +}
> +
>  static efi_status_t EFIAPI efi_open_protocol(
>                         void *handle, efi_guid_t *protocol,
>                         void **protocol_interface, void *agent_handle,
>                         void *controller_handle, uint32_t attributes)
>  {
> -       struct list_head *lhandle;
> -       int i;
> +       struct efi_handler *handler;
>         efi_status_t r = EFI_INVALID_PARAMETER;
>
>         EFI_ENTRY("%p, %p, %p, %p, %p, 0x%x", handle, protocol,
>                   protocol_interface, agent_handle, controller_handle,
>                   attributes);
>
> -       if (!handle || !protocol ||
> -           (!protocol_interface && attributes !=
> -            EFI_OPEN_PROTOCOL_TEST_PROTOCOL)) {
> +       if (!protocol_interface && attributes !=
> +           EFI_OPEN_PROTOCOL_TEST_PROTOCOL)
>                 goto out;
> -       }
>
>         switch (attributes) {
>         case EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL:
> @@ -1191,33 +1278,12 @@ static efi_status_t EFIAPI efi_open_protocol(
>                 goto out;
>         }
>
> -       list_for_each(lhandle, &efi_obj_list) {
> -               struct efi_object *efiobj;
> -               efiobj = list_entry(lhandle, struct efi_object, link);
> -
> -               if (efiobj->handle != handle)
> -                       continue;
> -
> -               for (i = 0; i < ARRAY_SIZE(efiobj->protocols); i++) {
> -                       struct efi_handler *handler = &efiobj->protocols[i];
> -                       const efi_guid_t *hprotocol = handler->guid;
> -                       if (!hprotocol)
> -                               continue;
> -                       if (!guidcmp(hprotocol, protocol)) {
> -                               if (attributes !=
> -                                   EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
> -                                       *protocol_interface =
> -                                               handler->protocol_interface;
> -                               }
> -                               r = EFI_SUCCESS;
> -                               goto out;
> -                       }
> -               }
> -               goto unsupported;
> -       }
> +       r = efi_search_protocol(handle, protocol, &handler);
> +       if (r != EFI_SUCCESS)
> +               goto out;
>
> -unsupported:
> -       r = EFI_UNSUPPORTED;
> +       r = efi_protocol_open(handler, protocol_interface, agent_handle,
> +                             controller_handle, attributes);
>  out:
>         return EFI_EXIT(r);
>  }
> --
> 2.14.1
>

[U-Boot] [PATCH 11/23] efi_loader: open_info in CloseProtocol

[Simon Glass]

On 27 August 2017 at 06:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> efi_open_protocol and efi_close_protocol have to keep track of
> opened protocols.
>
> efi_close_protocol has to mark the appropriate entry as
> empty.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_boottime.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)

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

Again I wonder if you can use a utility function to find the slot.

>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 294bc1f138..c9aec597a2 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -944,9 +944,40 @@ static efi_status_t EFIAPI efi_close_protocol(void *handle,
>                                               void *agent_handle,
>                                               void *controller_handle)
>  {
> +       struct efi_handler *handler;
> +       size_t i;
> +       struct efi_open_protocol_info_entry *open_info;
> +       efi_status_t r;
> +
>         EFI_ENTRY("%p, %p, %p, %p", handle, protocol, agent_handle,
>                   controller_handle);
> -       return EFI_EXIT(EFI_NOT_FOUND);
> +
> +       if (!agent_handle) {
> +               r = EFI_INVALID_PARAMETER;
> +               goto out;
> +       }
> +
> +       r = efi_search_protocol(handle, protocol, &handler);
> +       if (r != EFI_SUCCESS)
> +               goto out;
> +
> +       for (i = 0; i < ARRAY_SIZE(handler->open_info); ++i) {
> +               open_info = &handler->open_info[i];
> +
> +               if (!open_info->open_count)
> +                       continue;
> +
> +               if (open_info->agent_handle == agent_handle &&
> +                   open_info->controller_handle ==
> +                   controller_handle) {
> +                       open_info->open_count--;
> +                       r = EFI_SUCCESS;
> +                       goto out;
> +               }
> +       }
> +       r = EFI_NOT_FOUND;
> +out:
> +       return EFI_EXIT(r);
>  }
>
>  static efi_status_t EFIAPI efi_open_protocol_information(efi_handle_t handle,
> --
> 2.14.1
>

[U-Boot] [PATCH 12/23] efi_loader: implement OpenProtocolInformation

[Simon Glass]

Hi Heinrich,

On 27 August 2017 at 06:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> efi_open_protocol_information provides the agent and controller
> handles as well as the attributes and open count of an protocol
> on a handle.
>
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_boottime.c | 55 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 54 insertions(+), 1 deletion(-)

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

I can't help wondering if this would be better as a linked list?

>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index c9aec597a2..23b8894e73 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -985,9 +985,62 @@ static efi_status_t EFIAPI efi_open_protocol_information(efi_handle_t handle,
>                         struct efi_open_protocol_info_entry **entry_buffer,
>                         unsigned long *entry_count)
>  {
> +       unsigned long buffer_size;
> +       unsigned long count;
> +       struct efi_handler *handler;
> +       size_t i;
> +       efi_status_t r;
> +
>         EFI_ENTRY("%p, %p, %p, %p", handle, protocol, entry_buffer,
>                   entry_count);
> -       return EFI_EXIT(EFI_NOT_FOUND);
> +
> +       /* Check parameters */
> +       if (!handle || !protocol || !entry_buffer) {
> +               r = EFI_INVALID_PARAMETER;
> +               goto out;
> +       }
> +
> +       /* Find the protocol */
> +       r = efi_search_protocol(handle, protocol, &handler);
> +       if (r != EFI_SUCCESS)
> +               goto out;
> +
> +       *entry_buffer = NULL;
> +
> +       /* Count entries */
> +       count = 0;
> +       for (i = 0; i < ARRAY_SIZE(handler->open_info); ++i) {
> +               struct efi_open_protocol_info_entry *open_info =
> +                       &handler->open_info[i];
> +
> +               if (open_info->open_count)
> +                       ++count;
> +       }
> +       *entry_count = count;
> +       if (!count) {
> +               r = EFI_SUCCESS;
> +               goto out;
> +       }
> +
> +       /* Copy entries */
> +       buffer_size = count * sizeof(struct efi_open_protocol_info_entry);
> +       r = efi_allocate_pool(EFI_ALLOCATE_ANY_PAGES, buffer_size,
> +                             (void **)entry_buffer);
> +       if (r != EFI_SUCCESS)
> +               goto out;
> +       count = 0;
> +       for (i = 0; i < ARRAY_SIZE(handler->open_info); ++i) {
> +               struct efi_open_protocol_info_entry *open_info =
> +                       &handler->open_info[i];
> +
> +               if (!open_info->open_count)
> +                       continue;
> +               (*entry_buffer)[count] = *open_info;
> +               ++count;
> +       }
> +
> +out:
> +       return EFI_EXIT(r);
>  }
>
>  static efi_status_t EFIAPI efi_protocols_per_handle(void *handle,
> --
> 2.14.1
>

[U-Boot] [PATCH 13/23] efi_loader: non-static efi_open_protocol, efi_close_protocol

[Simon Glass]

On 27 August 2017 at 06:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> We need efi_open_protocol and efi_close_protocol for
> implementing other functions. So they shouldn't be static.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/efi_loader.h          | 9 +++++++++
>  lib/efi_loader/efi_boottime.c | 9 ++++-----
>  2 files changed, 13 insertions(+), 5 deletions(-)

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

But please document these two functions fully.

[U-Boot] [PATCH 14/23] efi_loader: pass GUIDs as const efi_guid_t *

[Simon Glass]

On 27 August 2017 at 06:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> We need to call some boottime services internally.
> Our GUIDs are stored as const efi_guid_t *.
>
> The boottime services never change GUIDs.
> So we can define the parameters as const efi_guid_t *.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/efi_api.h             | 44 +++++++++++++++++++++----------------------
>  include/efi_loader.h          |  4 ++--
>  lib/efi_loader/efi_boottime.c | 37 ++++++++++++++++++------------------
>  3 files changed, 43 insertions(+), 42 deletions(-)

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

Good.

[U-Boot] [PATCH 15/23] efi_loader: implement ConnectController

[Simon Glass]

On 27 August 2017 at 06:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/efi_api.h             |  22 ++++++++
>  include/efi_loader.h          |   1 +
>  lib/efi_loader/efi_boottime.c | 119 +++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 141 insertions(+), 1 deletion(-)
>

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

Please see below.

> diff --git a/include/efi_api.h b/include/efi_api.h
> index 8efc8dfab8..b2838125d7 100644
> --- a/include/efi_api.h
> +++ b/include/efi_api.h
> @@ -609,4 +609,26 @@ struct efi_pxe {
>         struct efi_pxe_mode *mode;
>  };
>
> +#define EFI_DRIVER_BINDING_PROTOCOL_GUID \
> +       EFI_GUID(0x18a031ab, 0xb443, 0x4d1a,\
> +                0xa5, 0xc0, 0x0c, 0x09, 0x26, 0x1e, 0x9f, 0x71)
> +struct efi_driver_binding_protocol {
> +       efi_status_t (EFIAPI * supported)(

I think the * should be up against 'supported'. Similarly below.

> +                       struct efi_driver_binding_protocol *this,
> +                       efi_handle_t controller_handle,
> +                       struct efi_device_path *remaining_device_path);
> +       efi_status_t (EFIAPI * start)(
> +                       struct efi_driver_binding_protocol *this,
> +                       efi_handle_t controller_handle,
> +                       struct efi_device_path *remaining_device_path);
> +       efi_status_t (EFIAPI * stop)(
> +                       struct efi_driver_binding_protocol *this,
> +                       efi_handle_t controller_handle,
> +                       UINTN number_of_children,
> +                       efi_handle_t child_handle_buffer);

These should have function comments.

> +       u32 version;
> +       efi_handle_t image_handle;
> +       efi_handle_t driver_binding_handle;
> +};
> +
>  #endif
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 9c68246c7c..f9f33e1d01 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -74,6 +74,7 @@ extern const struct efi_device_path_to_text_protocol efi_device_path_to_text;
>
>  extern const efi_guid_t efi_guid_console_control;
>  extern const efi_guid_t efi_guid_device_path;
> +extern const efi_guid_t efi_guid_driver_binding_protocol;

comment for this?

>  extern const efi_guid_t efi_guid_loaded_image;
>  extern const efi_guid_t efi_guid_device_path_to_text_protocol;
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 5a73ea5cd0..1069da7d79 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -18,6 +18,14 @@
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> +static efi_status_t EFIAPI efi_locate_protocol(const efi_guid_t *protocol,
> +                                              void *registration,
> +                                              void **protocol_interface);
> +static efi_status_t EFIAPI efi_locate_handle_buffer(
> +                       enum efi_locate_search_type search_type,
> +                       const efi_guid_t *protocol, void *search_key,
> +                       unsigned long *no_handles, efi_handle_t **buffer);
> +

Is this a forward decl? Can you reorder (in a separate patch) to avoid it?

[U-Boot] [PATCH 16/23] efi_loader: implement DisconnectController

[Simon Glass]

On 27 August 2017 at 06:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_boottime.c | 77 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 76 insertions(+), 1 deletion(-)
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 1069da7d79..c5a17b6252 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1052,9 +1052,84 @@ static efi_status_t EFIAPI efi_disconnect_controller(void *controller_handle,
>                                                      void *driver_image_handle,
>                                                      void *child_handle)

Can you add a function comment?

>  {
> +       struct efi_driver_binding_protocol *binding_protocol;
> +       efi_handle_t child_handle_buffer;
> +       unsigned long driver_count;
> +       efi_handle_t *driver_handle_buffer;
> +       size_t i;
> +       UINTN number_of_children;

Can we somehow use a lower-case type for this? Otherwise U-Boot will
start to look like EFI and I will have to start taking
antidepressants.

> +       efi_status_t r;
> +       size_t stop_count = 0;
> +
>         EFI_ENTRY("%p, %p, %p", controller_handle, driver_image_handle,
>                   child_handle);
> -       return EFI_EXIT(EFI_INVALID_PARAMETER);
> +
> +       if (!efi_search_obj(controller_handle)) {
> +               r = EFI_INVALID_PARAMETER;
> +               goto out;
> +       }
> +
> +       /* Create list of driver handles */
> +       if (driver_image_handle) {
> +               driver_handle_buffer = &driver_image_handle,
> +               driver_count = 1;
> +               /* Check that the handle supports driver binding protocol */
> +               r = efi_search_protocol(driver_image_handle,
> +                                       &efi_guid_driver_binding_protocol,
> +                                       NULL);
> +       } else {
> +               /* Get buffer with all handles with driver binding protocol */
> +               r = EFI_CALL(efi_locate_handle_buffer(
> +                            by_protocol, &efi_guid_driver_binding_protocol,
> +                            NULL, &driver_count, &driver_handle_buffer));
> +       }
> +       if (r != EFI_SUCCESS)
> +               goto out;
> +
> +       /* Create list of child handles */
> +       if (child_handle) {
> +               number_of_children = 1;
> +               child_handle_buffer = &child_handle;
> +       } else {
> +               /*
> +                * We do not fully support child handles.
> +                *
> +                * It is unclear from which handle and which protocols the
> +                * list of child controllers should be collected.
> +                */
> +               number_of_children = 0;
> +               child_handle_buffer = NULL;
> +       }
> +
> +       for (i = 0; i < driver_count; ++i) {
> +               r = EFI_CALL(efi_open_protocol(
> +                            driver_handle_buffer[i],
> +                            &efi_guid_driver_binding_protocol,
> +                            (void **)&binding_protocol,
> +                            driver_handle_buffer[i], NULL,
> +                            EFI_OPEN_PROTOCOL_GET_PROTOCOL));
> +               if (r != EFI_SUCCESS)
> +                       continue;
> +
> +               r = EFI_CALL(binding_protocol->stop(binding_protocol,
> +                                                   controller_handle,
> +                                                   number_of_children,
> +                                                   child_handle_buffer));
> +               if (r == EFI_SUCCESS)
> +                       ++stop_count;
> +               EFI_CALL(efi_close_protocol(driver_handle_buffer[i],
> +                                           &efi_guid_driver_binding_protocol,
> +                                           driver_handle_buffer[i], NULL));
> +       }
> +
> +       if (driver_image_handle)
> +               efi_free_pool(driver_handle_buffer);
> +       if (stop_count)
> +               r = EFI_SUCCESS;
> +       else
> +               r = EFI_NOT_FOUND;
> +out:
> +       return EFI_EXIT(r);
>  }
>
>  efi_status_t EFIAPI efi_close_protocol(void *handle, const efi_guid_t *protocol,
> --
> 2.14.1
>

[U-Boot] [PATCH 17/23] efi_loader: efi_net: hwaddr_size = 6

[Simon Glass]

On 27 August 2017 at 06:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> The length of a MAC address is 6.
> We have to set this length in the EFI_SIMPLE_NETWORK_MODE
> structure of the EFI_SIMPLE_NETWORK_PROTOCOL.
>
> Without this patch iPXE fails to initialize the network with
> error message
> SNP MAC(001e0633bcbf,0x0) has invalid hardware address length 0
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_net.c | 1 +
>  1 file changed, 1 insertion(+)

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

Can you use ARP_HLEN?

>
> diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
> index 0b949d86e8..75d7974b0e 100644
> --- a/lib/efi_loader/efi_net.c
> +++ b/lib/efi_loader/efi_net.c
> @@ -259,6 +259,7 @@ int efi_net_register(void **handle)
>         netobj->dp_end = dp_end;
>         memcpy(netobj->dp_mac.mac.addr, eth_get_ethaddr(), 6);
>         memcpy(netobj->net_mode.current_address.mac_addr, eth_get_ethaddr(), 6);
> +       netobj->net_mode.hwaddr_size = 6;
>         netobj->net_mode.max_packet_size = PKTSIZE;
>
>         netobj->pxe.mode = &netobj->pxe_mode;
> --
> 2.14.1
>

[U-Boot] [PATCH 18/23] efi_net: return EFI_UNSUPPORTED where appropriate

[Simon Glass]

On 27 August 2017 at 06:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> U-Boot does not implement all functions of the simple network
> protocol. The unimplemented functions return either of
> EFI_SUCCESS and EFI_INVALID_PARAMETER.
>
> The UEFI spec foresees to return EFI_UNSUPPORTED in these cases.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_net.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)

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

[U-Boot] [PATCH 19/23] efi_loader: correct bits of receive_filters bit mask

[Simon Glass]

On 27 August 2017 at 06:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Remove extraneous commas.
> Add comment.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/efi_api.h | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

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

[U-Boot] [PATCH 20/23] efi_loader: use events for efi_net_receive

[Simon Glass]

On 27 August 2017 at 06:54, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> A timer event is defined. The timer handler cares for receiving new
> packets.
>
> efi_timer_check is called both in efi_net_transmit and efi_net_receive
> to enable events during network communication.
>
> Calling efi_timer_check in efi_net_get_status is implemented in a
> separate patch.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_net.c | 35 ++++++++++++++++++++++++++++++++---
>  1 file changed, 32 insertions(+), 3 deletions(-)
>

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

But this code should not support pre-DM networking. E.g. calling eth_rx()...

It should be adjusted to work only with boards that define
CONFIG_DM_ETH. Otherwise we will never be able to move things over.

[U-Boot] [PATCH 21/23] efi_loader: fix efi_net_get_status

[Simon Glass]

On 27 August 2017 at 06:54, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> The returned interrupt status was wrong.
>
> As out transmit buffer is empty we need to always set
> EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT.
>
> When we have received a packet we need to set
> EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT.
>
> Furthermore we should call efi_timer_check() to handle events.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/efi_api.h        |  6 ++++++
>  lib/efi_loader/efi_net.c | 11 ++++++++---
>  2 files changed, 14 insertions(+), 3 deletions(-)

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

[U-Boot] [PATCH 22/23] efi_loader: set parent handle in efi_load_image

[Simon Glass]

On 27 August 2017 at 06:54, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> The parent_handle of the loaded image must be set.
> Add the file path protocol from the provided parameter.
> Set system table.
> Add parameter checks.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_boottime.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
>

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

A function comment on this would be nice

[U-Boot] [PATCH 23/23] efi_loader: implement SetWatchdogTimer

[Simon Glass]

On 27 August 2017 at 06:54, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> The watchdog is initialized with a 5 minute timeout period.
> It can be reset by SetWatchdogTimer.
> It is stopped by ExitBoottimeServices.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  cmd/bootefi.c                 |  1 +
>  include/efi_loader.h          |  4 +++
>  lib/efi_loader/Makefile       |  2 +-
>  lib/efi_loader/efi_boottime.c |  3 ++-
>  lib/efi_loader/efi_watchdog.c | 58 +++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 66 insertions(+), 2 deletions(-)
>  create mode 100644 lib/efi_loader/efi_watchdog.c
>

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

I'm wondering whether the efi_ prefix on all these files is
superfluous. What is the purpose?

[U-Boot] [PATCH 01/23] efi_loader: allow return value in EFI_CALL

[Alexander Graf]

On 08/31/2017 02:51 PM, Simon Glass wrote:
> On 27 August 2017 at 06:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> Macro EFI_CALL was introduced to call an UEFI function.
> Should this be 'an EFI'. Or 'a UEFI'?

In a nutshell, EFI is the v1, proprietary, for pay, closed protocol 
while UEFI is v2 and what edk2 implements, what you can get specs for, 
etc. But since people started implementing things for v1 back in the day 
and they are reasonable compatible, nobody calls UEFI UEFI, but just EFI :).

It's a mess and I certainly didn't help it by calling everything EFI, 
but it's what we're in and we should rather stay coherent IMHO. So 
EFI_CALL really does call UEFI functions ;).


Alex

[U-Boot] [PATCH 03/23] efi_loader: support 16 protocols per efi_object

[Alexander Graf]

On 08/27/2017 12:51 AM, Heinrich Schuchardt wrote:
> 8 protocols per efi_object is insufficient for iPXE.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>   include/efi_loader.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 6f71a6202b..e8fb4fbb0a 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -99,8 +99,8 @@ struct efi_handler {
>   struct efi_object {
>   	/* Every UEFI object is part of a global object list */
>   	struct list_head link;
> -	/* We support up to 8 "protocols" an object can be accessed through */
> -	struct efi_handler protocols[8];
> +	/* We support up to 16 "protocols" an object can be accessed through */
> +	struct efi_handler protocols[16];

Can you try to convert it into a list instead? Leif tried to make the 
UEFI Shell work and stumbled over the same limitation, so I guess a 
static array won't cut it for long.


Alex

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Leif Lindholm]

On Wed, Aug 30, 2017 at 08:59:31AM +0100, Leif Lindholm wrote:
> On Wed, Aug 30, 2017 at 12:03:16AM +0200, Heinrich Schuchardt wrote:
> > I am able to provide a test application that will cover the API
> > functions that I have focused on (events, protocols, (dis)connect
> > controllers). To limit my effort I would like to write it for the HEAD
> > of my patch queue and not break it down into one test patch per
> > implementation patch. I hope that is ok for you. My effort estimate is
> > 40h+ so, please, be patient.
> 
> I am not going to claim that getting SCT to run under U-Boot is going
> to come near the top of my priority queue any time soon, and it would
> certainly be useful for some sort of "make sure we don't break what we
> have" tests.

Well, I spent a few hours looking at what the immediate issues may be
with running the UEFI Shell (built with -D NO_SHELL_PROFILES), same as
edk2/ShellBinPkg/MinUefiShell. Alex suggested I post an update.

I've implemented stubs for the missing protocols preventing the Shell
from starting properly, and pushed everything to
https://git.linaro.org/people/leif.lindholm/u-boot.git/log/?h=uefi-shell

(As Alex points out, I forgot about the EFI_EXIT/EFI_ENTER macros.)

With this, execution fails with
Found 2 disks
new_package_list: 99

ASSERT_EFI_ERROR (Status = Device Error)
ASSERT [Shell]
/work/git/edk2/Build/Shell/DEBUG_GCC5/AARCH64/ShellPkg/Application/Shell/Shell/DEBUG/AutoGen.c(615):
!EFI_ERROR (Status)
"Synchronous Abort" handler, esr 0x5600dbdb

The AutoGen.c failure is a library constructor
(ShellLevel2CommandsLibConstructor) returning error because
MdeModulePkg/Library/UefiHiiLib/HiiLib.c : HiiAddPackages() received
an error return from new_package_list (in the non-camel-case u-boot
form).

If you could look into implementing something useful in that function,
I could look into what causes the next stumbling block after that.

I did overrun the "maximum number of open protocols" limit, so
randomly bumped that up to 12. It will need to go higher (and
preferably dynamic). As long as we're not planning to support
protocols staying around at runtime, I'd say that shouldn't be too
hard to resolve properly.

I have pushed an update to the pre-built versions of UEFI Shell in edk2:
https://github.com/tianocore/edk2/tree/master/ShellBinPkg/MinUefiShell
These are built without ASSERTs, so won't actually give the above
messages, but should otherwise be identical to the one I've used here.

/
    Leif

[U-Boot] [PATCH 12/23] efi_loader: implement OpenProtocolInformation

[Heinrich Schuchardt]

On 08/31/2017 02:51 PM, Simon Glass wrote:
> Hi Heinrich,
> 
> On 27 August 2017 at 06:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> efi_open_protocol_information provides the agent and controller
>> handles as well as the attributes and open count of an protocol
>> on a handle.
>>
>> Cc: Rob Clark <robdclark@gmail.com>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  lib/efi_loader/efi_boottime.c | 55 ++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 54 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> I can't help wondering if this would be better as a linked list?

This is an API function. The interface of the function has to be kept
the way it is.

Internally we could use other storage models.

Best regards

Heinrich

> 
>>
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index c9aec597a2..23b8894e73 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -985,9 +985,62 @@ static efi_status_t EFIAPI efi_open_protocol_information(efi_handle_t handle,
>>                         struct efi_open_protocol_info_entry **entry_buffer,
>>                         unsigned long *entry_count)
>>  {
>> +       unsigned long buffer_size;
>> +       unsigned long count;
>> +       struct efi_handler *handler;
>> +       size_t i;
>> +       efi_status_t r;
>> +
>>         EFI_ENTRY("%p, %p, %p, %p", handle, protocol, entry_buffer,
>>                   entry_count);
>> -       return EFI_EXIT(EFI_NOT_FOUND);
>> +
>> +       /* Check parameters */
>> +       if (!handle || !protocol || !entry_buffer) {
>> +               r = EFI_INVALID_PARAMETER;
>> +               goto out;
>> +       }
>> +
>> +       /* Find the protocol */
>> +       r = efi_search_protocol(handle, protocol, &handler);
>> +       if (r != EFI_SUCCESS)
>> +               goto out;
>> +
>> +       *entry_buffer = NULL;
>> +
>> +       /* Count entries */
>> +       count = 0;
>> +       for (i = 0; i < ARRAY_SIZE(handler->open_info); ++i) {
>> +               struct efi_open_protocol_info_entry *open_info =
>> +                       &handler->open_info[i];
>> +
>> +               if (open_info->open_count)
>> +                       ++count;
>> +       }
>> +       *entry_count = count;
>> +       if (!count) {
>> +               r = EFI_SUCCESS;
>> +               goto out;
>> +       }
>> +
>> +       /* Copy entries */
>> +       buffer_size = count * sizeof(struct efi_open_protocol_info_entry);
>> +       r = efi_allocate_pool(EFI_ALLOCATE_ANY_PAGES, buffer_size,
>> +                             (void **)entry_buffer);
>> +       if (r != EFI_SUCCESS)
>> +               goto out;
>> +       count = 0;
>> +       for (i = 0; i < ARRAY_SIZE(handler->open_info); ++i) {
>> +               struct efi_open_protocol_info_entry *open_info =
>> +                       &handler->open_info[i];
>> +
>> +               if (!open_info->open_count)
>> +                       continue;
>> +               (*entry_buffer)[count] = *open_info;
>> +               ++count;
>> +       }
>> +
>> +out:
>> +       return EFI_EXIT(r);
>>  }
>>
>>  static efi_status_t EFIAPI efi_protocols_per_handle(void *handle,
>> --
>> 2.14.1
>>
> 

[U-Boot] [PATCH 03/23] efi_loader: support 16 protocols per efi_object

[Heinrich Schuchardt]

On 08/31/2017 04:01 PM, Alexander Graf wrote:
> On 08/27/2017 12:51 AM, Heinrich Schuchardt wrote:
>> 8 protocols per efi_object is insufficient for iPXE.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>   include/efi_loader.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 6f71a6202b..e8fb4fbb0a 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -99,8 +99,8 @@ struct efi_handler {
>>   struct efi_object {
>>       /* Every UEFI object is part of a global object list */
>>       struct list_head link;
>> -    /* We support up to 8 "protocols" an object can be accessed
>> through */
>> -    struct efi_handler protocols[8];
>> +    /* We support up to 16 "protocols" an object can be accessed
>> through */
>> +    struct efi_handler protocols[16];
> 
> Can you try to convert it into a list instead? Leif tried to make the
> UEFI Shell work and stumbled over the same limitation, so I guess a
> static array won't cut it for long.
> 
> 
> Alex
> 
> 

Hello Alex,

is it safe to call malloc and free before efi_exit_boot_services?

Currently we do not check that boottime services are not called after
efi_exit_boot_services. Shouldn't every call to boottime services fail
thereafter? The spec says: "After successfully calling
ExitBootServices(), all boot services in the system are terminated."

We definitively do not want to call malloc at runtime because all
available memory (except for EFI_MEMORY_RUNTIME) is managed by the
operating system.

Best regards

Heinrich

[U-Boot] [PATCH 03/23] efi_loader: support 16 protocols per efi_object

[Alexander Graf]

On 01.09.17 03:45, Heinrich Schuchardt wrote:
> On 08/31/2017 04:01 PM, Alexander Graf wrote:
>> On 08/27/2017 12:51 AM, Heinrich Schuchardt wrote:
>>> 8 protocols per efi_object is insufficient for iPXE.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> ---
>>>    include/efi_loader.h | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>> index 6f71a6202b..e8fb4fbb0a 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -99,8 +99,8 @@ struct efi_handler {
>>>    struct efi_object {
>>>        /* Every UEFI object is part of a global object list */
>>>        struct list_head link;
>>> -    /* We support up to 8 "protocols" an object can be accessed
>>> through */
>>> -    struct efi_handler protocols[8];
>>> +    /* We support up to 16 "protocols" an object can be accessed
>>> through */
>>> +    struct efi_handler protocols[16];
>>
>> Can you try to convert it into a list instead? Leif tried to make the
>> UEFI Shell work and stumbled over the same limitation, so I guess a
>> static array won't cut it for long.
>>
>>
>> Alex
>>
>>
> 
> Hello Alex,
> 
> is it safe to call malloc and free before efi_exit_boot_services?

Yes :). Before exiting boot services we're basically in normal U-Boot 
space with a U-Boot owned malloc region that we can allocate from.

> Currently we do not check that boottime services are not called after
> efi_exit_boot_services. Shouldn't every call to boottime services fail

Yes, IIUC it's simply illegal to call them afterwards.

> thereafter? The spec says: "After successfully calling
> ExitBootServices(), all boot services in the system are terminated."

I'm not sure. I'd be very surprised to see a payload actually call any 
boot service after exiting boot services. Runtime services is a 
different question, but those are very special anyway.

> We definitively do not want to call malloc at runtime because all
> available memory (except for EFI_MEMORY_RUNTIME) is managed by the
> operating system.

Yes, but efi objects only exist during boot time. Runtime services don't 
expose protocols or objects.


Alex

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Rob Clark]

On Thu, Aug 31, 2017 at 10:45 AM, Leif Lindholm
<leif.lindholm@linaro.org> wrote:
> On Wed, Aug 30, 2017 at 08:59:31AM +0100, Leif Lindholm wrote:
>> On Wed, Aug 30, 2017 at 12:03:16AM +0200, Heinrich Schuchardt wrote:
>> > I am able to provide a test application that will cover the API
>> > functions that I have focused on (events, protocols, (dis)connect
>> > controllers). To limit my effort I would like to write it for the HEAD
>> > of my patch queue and not break it down into one test patch per
>> > implementation patch. I hope that is ok for you. My effort estimate is
>> > 40h+ so, please, be patient.
>>
>> I am not going to claim that getting SCT to run under U-Boot is going
>> to come near the top of my priority queue any time soon, and it would
>> certainly be useful for some sort of "make sure we don't break what we
>> have" tests.
>
> Well, I spent a few hours looking at what the immediate issues may be
> with running the UEFI Shell (built with -D NO_SHELL_PROFILES), same as
> edk2/ShellBinPkg/MinUefiShell. Alex suggested I post an update.
>
> I've implemented stubs for the missing protocols preventing the Shell
> from starting properly, and pushed everything to
> https://git.linaro.org/people/leif.lindholm/u-boot.git/log/?h=uefi-shell

from a quick look, at least we have part of the under-the-hood bits
for "efi_loader: add stub EFI_DEVICE_PATH_UTILITIES_PROTOCOL" in:

https://github.com/robclark/u-boot/commit/a00519e2fa7b716dda717a24799a0629d42eb362

I can take a stab at fleshing this one out over the weekend..

BR,
-R

> (As Alex points out, I forgot about the EFI_EXIT/EFI_ENTER macros.)
>
> With this, execution fails with
> Found 2 disks
> new_package_list: 99
>
> ASSERT_EFI_ERROR (Status = Device Error)
> ASSERT [Shell]
> /work/git/edk2/Build/Shell/DEBUG_GCC5/AARCH64/ShellPkg/Application/Shell/Shell/DEBUG/AutoGen.c(615):
> !EFI_ERROR (Status)
> "Synchronous Abort" handler, esr 0x5600dbdb
>
> The AutoGen.c failure is a library constructor
> (ShellLevel2CommandsLibConstructor) returning error because
> MdeModulePkg/Library/UefiHiiLib/HiiLib.c : HiiAddPackages() received
> an error return from new_package_list (in the non-camel-case u-boot
> form).
>
> If you could look into implementing something useful in that function,
> I could look into what causes the next stumbling block after that.
>
> I did overrun the "maximum number of open protocols" limit, so
> randomly bumped that up to 12. It will need to go higher (and
> preferably dynamic). As long as we're not planning to support
> protocols staying around at runtime, I'd say that shouldn't be too
> hard to resolve properly.
>
> I have pushed an update to the pre-built versions of UEFI Shell in edk2:
> https://github.com/tianocore/edk2/tree/master/ShellBinPkg/MinUefiShell
> These are built without ASSERTs, so won't actually give the above
> messages, but should otherwise be identical to the one I've used here.
>
> /
>     Leif

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Rob Clark]

On Thu, Aug 31, 2017 at 10:45 AM, Leif Lindholm
<leif.lindholm@linaro.org> wrote:
> On Wed, Aug 30, 2017 at 08:59:31AM +0100, Leif Lindholm wrote:
>> On Wed, Aug 30, 2017 at 12:03:16AM +0200, Heinrich Schuchardt wrote:
>> > I am able to provide a test application that will cover the API
>> > functions that I have focused on (events, protocols, (dis)connect
>> > controllers). To limit my effort I would like to write it for the HEAD
>> > of my patch queue and not break it down into one test patch per
>> > implementation patch. I hope that is ok for you. My effort estimate is
>> > 40h+ so, please, be patient.
>>
>> I am not going to claim that getting SCT to run under U-Boot is going
>> to come near the top of my priority queue any time soon, and it would
>> certainly be useful for some sort of "make sure we don't break what we
>> have" tests.
>
> Well, I spent a few hours looking at what the immediate issues may be
> with running the UEFI Shell (built with -D NO_SHELL_PROFILES), same as
> edk2/ShellBinPkg/MinUefiShell. Alex suggested I post an update.
>
> I've implemented stubs for the missing protocols preventing the Shell
> from starting properly, and pushed everything to
> https://git.linaro.org/people/leif.lindholm/u-boot.git/log/?h=uefi-shell

from a quick look, at least we have part of the under-the-hood bits
for "efi_loader: add stub EFI_DEVICE_PATH_UTILITIES_PROTOCOL" in:

https://github.com/robclark/u-boot/commit/a00519e2fa7b716dda717a24799a0629d42eb362

I can take a stab at fleshing this one out over the weekend..

BR,
-R

> (As Alex points out, I forgot about the EFI_EXIT/EFI_ENTER macros.)
>
> With this, execution fails with
> Found 2 disks
> new_package_list: 99
>
> ASSERT_EFI_ERROR (Status = Device Error)
> ASSERT [Shell]
> /work/git/edk2/Build/Shell/DEBUG_GCC5/AARCH64/ShellPkg/Application/Shell/Shell/DEBUG/AutoGen.c(615):
> !EFI_ERROR (Status)
> "Synchronous Abort" handler, esr 0x5600dbdb
>
> The AutoGen.c failure is a library constructor
> (ShellLevel2CommandsLibConstructor) returning error because
> MdeModulePkg/Library/UefiHiiLib/HiiLib.c : HiiAddPackages() received
> an error return from new_package_list (in the non-camel-case u-boot
> form).
>
> If you could look into implementing something useful in that function,
> I could look into what causes the next stumbling block after that.
>
> I did overrun the "maximum number of open protocols" limit, so
> randomly bumped that up to 12. It will need to go higher (and
> preferably dynamic). As long as we're not planning to support
> protocols staying around at runtime, I'd say that shouldn't be too
> hard to resolve properly.
>
> I have pushed an update to the pre-built versions of UEFI Shell in edk2:
> https://github.com/tianocore/edk2/tree/master/ShellBinPkg/MinUefiShell
> These are built without ASSERTs, so won't actually give the above
> messages, but should otherwise be identical to the one I've used here.
>
> /
>     Leif

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Tom Rini]

On Wed, Aug 30, 2017 at 04:16:34AM +0800, Simon Glass wrote:
> Hi,
> 
> On 29 August 2017 at 22:16, Rob Clark <robdclark@gmail.com> wrote:
> > On Tue, Aug 29, 2017 at 8:57 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >> On Tue, Aug 29, 2017 at 02:26:48PM +0200, Alexander Graf wrote:
> >>> > > > I would add command
> >>> > > > bootefi selftest.efi
> >>> > > > to run the tests and provide the python wrapper code to add it to the
> >>> > > > test suite.
> >>> > >
> >>> > > I think that's a great idea, yes.
> >>> > I wonder how far we are from running UEFI tests (either the official
> >>> > ones, or I seem to remember hearing about some other test suite which
> >>> > didn't require UEFI shell)?
> >>>
> >>> Let's ask Leif, Ard and Dong.
> >>>
> >>> The official test suite definitely needs the UEFI Shell. Is the suite
> >>> publicly available by now?
> >>
> >> In binary form, you can access it already from the links on
> >> http://uefi.org/testtools
> >>
> >> Yes, 2.5 is latest release. No this is not a restriction ... the SCT
> >> releases have been lagging the specification releases a fair bit.
> >>
> >> The 2.5a package contains AARCH64, IA32 and X64 support (not ARM).
> >> Not that it couldn't contain ARM, it just hasn't been packaged.
> >>
> >>> > That seems more useful long term than re-inventing comprehensive UEFI
> >>> > test suite.  (Also, beyond just running shim/fallback/grub, I don't
> >>> > really have time to invent new tests for the stack of efi_loader
> >>> > patches I have.)
> >>>
> >>> Yes and no - it depends on the availability of the official suite :/.
> >>
> >> UEFI SCT is not yet open source/free software. Obviously, this is
> >> something Linaro has been lobbying for since day one of our
> >> involvement. There used to be little understanding for this, but that
> >> attitude has shifted substantially.
> >
> > So, if/until UEFI SCT is not an option, what about:
> >
> >   https://01.org/linux-uefi-validation
> >
> > (thx to pjones for pointing that out to me)
> 
> Well in any case I'm not looking for a large functional test suite at
> this stage. It certainly could be useful, but not as a replacement for
> unit tests. The latter is for fast verification (so that everyone can
> run it as part of 'make tests') and easy identification of the
> location of bugs.

I want to chime in here.  Unless we're talking hours of time, if "make
tests" takes 5 minutes to run, but that includes doing some sort of full
validation suite to the relevant EFI code, that seems like a win to me.
And if someone else is responsible for the contents of the tests and we
just have to confirm our expected results, that's an even bigger win.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170901/6b47ad1d/attachment.sig>

[U-Boot] [PATCH 03/23] efi_loader: support 16 protocols per efi_object

[Rob Clark]

On Thu, Aug 31, 2017 at 10:01 AM, Alexander Graf <agraf@suse.de> wrote:
> On 08/27/2017 12:51 AM, Heinrich Schuchardt wrote:
>>
>> 8 protocols per efi_object is insufficient for iPXE.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>   include/efi_loader.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 6f71a6202b..e8fb4fbb0a 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -99,8 +99,8 @@ struct efi_handler {
>>   struct efi_object {
>>         /* Every UEFI object is part of a global object list */
>>         struct list_head link;
>> -       /* We support up to 8 "protocols" an object can be accessed
>> through */
>> -       struct efi_handler protocols[8];
>> +       /* We support up to 16 "protocols" an object can be accessed
>> through */
>> +       struct efi_handler protocols[16];
>
>
> Can you try to convert it into a list instead? Leif tried to make the UEFI
> Shell work and stumbled over the same limitation, so I guess a static array
> won't cut it for long.
>

Can we go w/ fixed 16 protocols length for now?  A list is a
definitely a better option, and it will be easier after "efi_loader:
refactor boot device and loaded_image handling" (which gets rid of the
statically initialized efi_object's).  After that we can drop the
fixed length array and add an 'void append_protocol(efiobj, guid,
handle)'  helper fairly easily.

BR,
-R

[U-Boot] [PATCH 03/23] efi_loader: support 16 protocols per efi_object

[Alexander Graf]

> Am 02.09.2017 um 20:14 schrieb Rob Clark <robdclark@gmail.com>:
> 
>> On Thu, Aug 31, 2017 at 10:01 AM, Alexander Graf <agraf@suse.de> wrote:
>>> On 08/27/2017 12:51 AM, Heinrich Schuchardt wrote:
>>> 
>>> 8 protocols per efi_object is insufficient for iPXE.
>>> 
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> ---
>>>  include/efi_loader.h | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>> index 6f71a6202b..e8fb4fbb0a 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -99,8 +99,8 @@ struct efi_handler {
>>>  struct efi_object {
>>>        /* Every UEFI object is part of a global object list */
>>>        struct list_head link;
>>> -       /* We support up to 8 "protocols" an object can be accessed
>>> through */
>>> -       struct efi_handler protocols[8];
>>> +       /* We support up to 16 "protocols" an object can be accessed
>>> through */
>>> +       struct efi_handler protocols[16];
>> 
>> 
>> Can you try to convert it into a list instead? Leif tried to make the UEFI
>> Shell work and stumbled over the same limitation, so I guess a static array
>> won't cut it for long.
>> 
> 
> Can we go w/ fixed 16 protocols length for now?  A list is a
> definitely a better option, and it will be easier after "efi_loader:
> refactor boot device and loaded_image handling" (which gets rid of the
> statically initialized efi_object's).  After that we can drop the
> fixed length array and add an 'void append_protocol(efiobj, guid,
> handle)'  helper fairly easily.

Sure, of course :)

Alex

> 
> BR,
> -R

[U-Boot] [PATCH 01/23] efi_loader: allow return value in EFI_CALL

[Simon Glass]

Hi Alex,

On 31 August 2017 at 21:58, Alexander Graf <agraf@suse.de> wrote:
>
> On 08/31/2017 02:51 PM, Simon Glass wrote:
>>
>> On 27 August 2017 at 06:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>
>>> Macro EFI_CALL was introduced to call an UEFI function.
>>
>> Should this be 'an EFI'. Or 'a UEFI'?
>
>
> In a nutshell, EFI is the v1, proprietary, for pay, closed protocol while UEFI is v2 and what edk2 implements, what you can get specs for, etc. But since people started implementing things for v1 back in the day and they are reasonable compatible, nobody calls UEFI UEFI, but just EFI :).
>
> It's a mess and I certainly didn't help it by calling everything EFI, but it's what we're in and we should rather stay coherent IMHO. So EFI_CALL really does call UEFI functions ;).

OK that's fine. Using EFI consistently seems like a good idea.

Regards,
Simon

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Simon Glass]

Hi,

On 1 September 2017 at 22:45, Tom Rini <trini@konsulko.com> wrote:
> On Wed, Aug 30, 2017 at 04:16:34AM +0800, Simon Glass wrote:
>> Hi,
>>
>> On 29 August 2017 at 22:16, Rob Clark <robdclark@gmail.com> wrote:
>> > On Tue, Aug 29, 2017 at 8:57 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> >> On Tue, Aug 29, 2017 at 02:26:48PM +0200, Alexander Graf wrote:
>> >>> > > > I would add command
>> >>> > > > bootefi selftest.efi
>> >>> > > > to run the tests and provide the python wrapper code to add it to the
>> >>> > > > test suite.
>> >>> > >
>> >>> > > I think that's a great idea, yes.
>> >>> > I wonder how far we are from running UEFI tests (either the official
>> >>> > ones, or I seem to remember hearing about some other test suite which
>> >>> > didn't require UEFI shell)?
>> >>>
>> >>> Let's ask Leif, Ard and Dong.
>> >>>
>> >>> The official test suite definitely needs the UEFI Shell. Is the suite
>> >>> publicly available by now?
>> >>
>> >> In binary form, you can access it already from the links on
>> >> http://uefi.org/testtools
>> >>
>> >> Yes, 2.5 is latest release. No this is not a restriction ... the SCT
>> >> releases have been lagging the specification releases a fair bit.
>> >>
>> >> The 2.5a package contains AARCH64, IA32 and X64 support (not ARM).
>> >> Not that it couldn't contain ARM, it just hasn't been packaged.
>> >>
>> >>> > That seems more useful long term than re-inventing comprehensive UEFI
>> >>> > test suite.  (Also, beyond just running shim/fallback/grub, I don't
>> >>> > really have time to invent new tests for the stack of efi_loader
>> >>> > patches I have.)
>> >>>
>> >>> Yes and no - it depends on the availability of the official suite :/.
>> >>
>> >> UEFI SCT is not yet open source/free software. Obviously, this is
>> >> something Linaro has been lobbying for since day one of our
>> >> involvement. There used to be little understanding for this, but that
>> >> attitude has shifted substantially.
>> >
>> > So, if/until UEFI SCT is not an option, what about:
>> >
>> >   https://01.org/linux-uefi-validation
>> >
>> > (thx to pjones for pointing that out to me)
>>
>> Well in any case I'm not looking for a large functional test suite at
>> this stage. It certainly could be useful, but not as a replacement for
>> unit tests. The latter is for fast verification (so that everyone can
>> run it as part of 'make tests') and easy identification of the
>> location of bugs.
>
> I want to chime in here.  Unless we're talking hours of time, if "make
> tests" takes 5 minutes to run, but that includes doing some sort of full
> validation suite to the relevant EFI code, that seems like a win to me.
> And if someone else is responsible for the contents of the tests and we
> just have to confirm our expected results, that's an even bigger win.

Yes it certainly is a win. But I'm already upset with how long the
tests take to run so I would not be keen on anything that increases it
much. How long does this test suite take to run normally?

Regards,
Simon

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Rob Clark]

On Tue, Sep 5, 2017 at 4:55 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi,
>
> On 1 September 2017 at 22:45, Tom Rini <trini@konsulko.com> wrote:
>> On Wed, Aug 30, 2017 at 04:16:34AM +0800, Simon Glass wrote:
>>> Hi,
>>>
>>> On 29 August 2017 at 22:16, Rob Clark <robdclark@gmail.com> wrote:
>>> > On Tue, Aug 29, 2017 at 8:57 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>> >> On Tue, Aug 29, 2017 at 02:26:48PM +0200, Alexander Graf wrote:
>>> >>> > > > I would add command
>>> >>> > > > bootefi selftest.efi
>>> >>> > > > to run the tests and provide the python wrapper code to add it to the
>>> >>> > > > test suite.
>>> >>> > >
>>> >>> > > I think that's a great idea, yes.
>>> >>> > I wonder how far we are from running UEFI tests (either the official
>>> >>> > ones, or I seem to remember hearing about some other test suite which
>>> >>> > didn't require UEFI shell)?
>>> >>>
>>> >>> Let's ask Leif, Ard and Dong.
>>> >>>
>>> >>> The official test suite definitely needs the UEFI Shell. Is the suite
>>> >>> publicly available by now?
>>> >>
>>> >> In binary form, you can access it already from the links on
>>> >> http://uefi.org/testtools
>>> >>
>>> >> Yes, 2.5 is latest release. No this is not a restriction ... the SCT
>>> >> releases have been lagging the specification releases a fair bit.
>>> >>
>>> >> The 2.5a package contains AARCH64, IA32 and X64 support (not ARM).
>>> >> Not that it couldn't contain ARM, it just hasn't been packaged.
>>> >>
>>> >>> > That seems more useful long term than re-inventing comprehensive UEFI
>>> >>> > test suite.  (Also, beyond just running shim/fallback/grub, I don't
>>> >>> > really have time to invent new tests for the stack of efi_loader
>>> >>> > patches I have.)
>>> >>>
>>> >>> Yes and no - it depends on the availability of the official suite :/.
>>> >>
>>> >> UEFI SCT is not yet open source/free software. Obviously, this is
>>> >> something Linaro has been lobbying for since day one of our
>>> >> involvement. There used to be little understanding for this, but that
>>> >> attitude has shifted substantially.
>>> >
>>> > So, if/until UEFI SCT is not an option, what about:
>>> >
>>> >   https://01.org/linux-uefi-validation
>>> >
>>> > (thx to pjones for pointing that out to me)
>>>
>>> Well in any case I'm not looking for a large functional test suite at
>>> this stage. It certainly could be useful, but not as a replacement for
>>> unit tests. The latter is for fast verification (so that everyone can
>>> run it as part of 'make tests') and easy identification of the
>>> location of bugs.
>>
>> I want to chime in here.  Unless we're talking hours of time, if "make
>> tests" takes 5 minutes to run, but that includes doing some sort of full
>> validation suite to the relevant EFI code, that seems like a win to me.
>> And if someone else is responsible for the contents of the tests and we
>> just have to confirm our expected results, that's an even bigger win.
>
> Yes it certainly is a win. But I'm already upset with how long the
> tests take to run so I would not be keen on anything that increases it
> much. How long does this test suite take to run normally?
>

I'm not sure how long SCT would take to run until we get it working (I
have a growing stack of patches on my wip-enough-uefi-for-shell
branch, and have Shell.efi running but still debugging sct.efi)..
maybe someone who has run SCT on a real UEFI implementation knows..
But that said, there seems to be some potential to run a subset of the
tests.  Either way, it seems like most of the time that travis runs
take is building a million variants of u-boot.  I'm not sure the time
spent running tests is really the issue.

BR,
-R

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Heinrich Schuchardt]

On 09/06/2017 01:48 AM, Rob Clark wrote:
> On Tue, Sep 5, 2017 at 4:55 AM, Simon Glass <sjg@chromium.org> wrote:
>> Hi,
>>
>> On 1 September 2017 at 22:45, Tom Rini <trini@konsulko.com> wrote:
>>> On Wed, Aug 30, 2017 at 04:16:34AM +0800, Simon Glass wrote:
>>>> Hi,
>>>>
>>>> On 29 August 2017 at 22:16, Rob Clark <robdclark@gmail.com> wrote:
>>>>> On Tue, Aug 29, 2017 at 8:57 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>>>>> On Tue, Aug 29, 2017 at 02:26:48PM +0200, Alexander Graf wrote:
>>>>>>>>>> I would add command
>>>>>>>>>> bootefi selftest.efi
>>>>>>>>>> to run the tests and provide the python wrapper code to add it to the
>>>>>>>>>> test suite.
>>>>>>>>>
>>>>>>>>> I think that's a great idea, yes.
>>>>>>>> I wonder how far we are from running UEFI tests (either the official
>>>>>>>> ones, or I seem to remember hearing about some other test suite which
>>>>>>>> didn't require UEFI shell)?
>>>>>>>
>>>>>>> Let's ask Leif, Ard and Dong.
>>>>>>>
>>>>>>> The official test suite definitely needs the UEFI Shell. Is the suite
>>>>>>> publicly available by now?
>>>>>>
>>>>>> In binary form, you can access it already from the links on
>>>>>> http://uefi.org/testtools
>>>>>>
>>>>>> Yes, 2.5 is latest release. No this is not a restriction ... the SCT
>>>>>> releases have been lagging the specification releases a fair bit.
>>>>>>
>>>>>> The 2.5a package contains AARCH64, IA32 and X64 support (not ARM).
>>>>>> Not that it couldn't contain ARM, it just hasn't been packaged.
>>>>>>
>>>>>>>> That seems more useful long term than re-inventing comprehensive UEFI
>>>>>>>> test suite.  (Also, beyond just running shim/fallback/grub, I don't
>>>>>>>> really have time to invent new tests for the stack of efi_loader
>>>>>>>> patches I have.)
>>>>>>>
>>>>>>> Yes and no - it depends on the availability of the official suite :/.
>>>>>>
>>>>>> UEFI SCT is not yet open source/free software. Obviously, this is
>>>>>> something Linaro has been lobbying for since day one of our
>>>>>> involvement. There used to be little understanding for this, but that
>>>>>> attitude has shifted substantially.
>>>>>
>>>>> So, if/until UEFI SCT is not an option, what about:
>>>>>
>>>>>   https://01.org/linux-uefi-validation
>>>>>
>>>>> (thx to pjones for pointing that out to me)
>>>>
>>>> Well in any case I'm not looking for a large functional test suite at
>>>> this stage. It certainly could be useful, but not as a replacement for
>>>> unit tests. The latter is for fast verification (so that everyone can
>>>> run it as part of 'make tests') and easy identification of the
>>>> location of bugs.
>>>
>>> I want to chime in here.  Unless we're talking hours of time, if "make
>>> tests" takes 5 minutes to run, but that includes doing some sort of full
>>> validation suite to the relevant EFI code, that seems like a win to me.
>>> And if someone else is responsible for the contents of the tests and we
>>> just have to confirm our expected results, that's an even bigger win.
>>
>> Yes it certainly is a win. But I'm already upset with how long the
>> tests take to run so I would not be keen on anything that increases it
>> much. How long does this test suite take to run normally?
>>
> 
> I'm not sure how long SCT would take to run until we get it working (I
> have a growing stack of patches on my wip-enough-uefi-for-shell
> branch, and have Shell.efi running but still debugging sct.efi)..
> maybe someone who has run SCT on a real UEFI implementation knows..
> But that said, there seems to be some potential to run a subset of the
> tests.  Either way, it seems like most of the time that travis runs
> take is building a million variants of u-boot.  I'm not sure the time
> spent running tests is really the issue.
> 
> BR,
> -R
> 
You could reduce the number of variants in .travis.yml by deleting
entries from env:.

With EFI the once that showed up my errors were:

qemu-x86
vexpress_ca15_tc2
vexpress_ca9x4

So maybe for a quick test (10 min) of your patches you can reduce to
these and do a full test before submitting.

Regards

Heinrich

[U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Rob Clark]

On Wed, Sep 6, 2017 at 12:18 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 09/06/2017 01:48 AM, Rob Clark wrote:
>> On Tue, Sep 5, 2017 at 4:55 AM, Simon Glass <sjg@chromium.org> wrote:
>>> Hi,
>>>
>>> On 1 September 2017 at 22:45, Tom Rini <trini@konsulko.com> wrote:
>>>> On Wed, Aug 30, 2017 at 04:16:34AM +0800, Simon Glass wrote:
>>>>> Hi,
>>>>>
>>>>> On 29 August 2017 at 22:16, Rob Clark <robdclark@gmail.com> wrote:
>>>>>> On Tue, Aug 29, 2017 at 8:57 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>>>>>> On Tue, Aug 29, 2017 at 02:26:48PM +0200, Alexander Graf wrote:
>>>>>>>>>>> I would add command
>>>>>>>>>>> bootefi selftest.efi
>>>>>>>>>>> to run the tests and provide the python wrapper code to add it to the
>>>>>>>>>>> test suite.
>>>>>>>>>>
>>>>>>>>>> I think that's a great idea, yes.
>>>>>>>>> I wonder how far we are from running UEFI tests (either the official
>>>>>>>>> ones, or I seem to remember hearing about some other test suite which
>>>>>>>>> didn't require UEFI shell)?
>>>>>>>>
>>>>>>>> Let's ask Leif, Ard and Dong.
>>>>>>>>
>>>>>>>> The official test suite definitely needs the UEFI Shell. Is the suite
>>>>>>>> publicly available by now?
>>>>>>>
>>>>>>> In binary form, you can access it already from the links on
>>>>>>> http://uefi.org/testtools
>>>>>>>
>>>>>>> Yes, 2.5 is latest release. No this is not a restriction ... the SCT
>>>>>>> releases have been lagging the specification releases a fair bit.
>>>>>>>
>>>>>>> The 2.5a package contains AARCH64, IA32 and X64 support (not ARM).
>>>>>>> Not that it couldn't contain ARM, it just hasn't been packaged.
>>>>>>>
>>>>>>>>> That seems more useful long term than re-inventing comprehensive UEFI
>>>>>>>>> test suite.  (Also, beyond just running shim/fallback/grub, I don't
>>>>>>>>> really have time to invent new tests for the stack of efi_loader
>>>>>>>>> patches I have.)
>>>>>>>>
>>>>>>>> Yes and no - it depends on the availability of the official suite :/.
>>>>>>>
>>>>>>> UEFI SCT is not yet open source/free software. Obviously, this is
>>>>>>> something Linaro has been lobbying for since day one of our
>>>>>>> involvement. There used to be little understanding for this, but that
>>>>>>> attitude has shifted substantially.
>>>>>>
>>>>>> So, if/until UEFI SCT is not an option, what about:
>>>>>>
>>>>>>   https://01.org/linux-uefi-validation
>>>>>>
>>>>>> (thx to pjones for pointing that out to me)
>>>>>
>>>>> Well in any case I'm not looking for a large functional test suite at
>>>>> this stage. It certainly could be useful, but not as a replacement for
>>>>> unit tests. The latter is for fast verification (so that everyone can
>>>>> run it as part of 'make tests') and easy identification of the
>>>>> location of bugs.
>>>>
>>>> I want to chime in here.  Unless we're talking hours of time, if "make
>>>> tests" takes 5 minutes to run, but that includes doing some sort of full
>>>> validation suite to the relevant EFI code, that seems like a win to me.
>>>> And if someone else is responsible for the contents of the tests and we
>>>> just have to confirm our expected results, that's an even bigger win.
>>>
>>> Yes it certainly is a win. But I'm already upset with how long the
>>> tests take to run so I would not be keen on anything that increases it
>>> much. How long does this test suite take to run normally?
>>>
>>
>> I'm not sure how long SCT would take to run until we get it working (I
>> have a growing stack of patches on my wip-enough-uefi-for-shell
>> branch, and have Shell.efi running but still debugging sct.efi)..
>> maybe someone who has run SCT on a real UEFI implementation knows..
>> But that said, there seems to be some potential to run a subset of the
>> tests.  Either way, it seems like most of the time that travis runs
>> take is building a million variants of u-boot.  I'm not sure the time
>> spent running tests is really the issue.
>>
>> BR,
>> -R
>>
> You could reduce the number of variants in .travis.yml by deleting
> entries from env:.

yeah, this is something I've done before to try to debug issues that
only show up in travis and that I could not reproduce locally

> With EFI the once that showed up my errors were:
>
> qemu-x86
> vexpress_ca15_tc2
> vexpress_ca9x4

imho, we need something aarch64 and using DM in the mix.. and so far
I've only seem the same issues on both vexpress platforms, so I'm not
sure how useful it is to test both (at least for a quick test run)

BR,
-R

> So maybe for a quick test (10 min) of your patches you can reduce to
> these and do a full test before submitting.
>
> Regards
>
> Heinrich

[U-Boot] [PATCH 08/23] efi_loader: allow creating new handles

[Rob Clark]

On Thu, Aug 31, 2017 at 8:51 AM, Simon Glass <sjg@chromium.org> wrote:
> On 27 August 2017 at 06:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> In efi_install_protocol_interface support creating
>> a new handle.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  lib/efi_loader/efi_boottime.c | 22 ++++++++++++++++++++--
>>  1 file changed, 20 insertions(+), 2 deletions(-)
>>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> The use of void ** seems odd. Is that mandated by EFI? Is there no way
> to use a proper type?

There is in fact an efi_handle_t, which we should probably start using..

Anyways, turns out SCT needed this patch, so Heinrich, you can add my:

Tested-by: Rob Clark <robdclark@gmail.com>


>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index 96cb1fa410..9f8d64659f 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -238,6 +238,23 @@ static efi_status_t EFIAPI efi_free_pool_ext(void *buffer)
>>         return EFI_EXIT(r);
>>  }
>>
>> +static efi_status_t efi_create_handle(void **handle)
>> +{
>> +       struct efi_object *obj;
>> +       efi_status_t r;
>> +
>> +       r = efi_allocate_pool(EFI_ALLOCATE_ANY_PAGES,
>> +                             sizeof(struct efi_object),
>> +                             (void **)&obj);
>> +       if (r != EFI_SUCCESS)
>> +               return r;
>> +       memset(obj, 0, sizeof(struct efi_object));
>> +       obj->handle = obj;
>> +       list_add_tail(&obj->link, &efi_obj_list);
>> +       *handle = obj;
>> +       return r;
>> +}
>> +
>>  /*
>>   * Our event capabilities are very limited. Only a small limited
>>   * number of events is allowed to coexist.
>> @@ -497,8 +514,9 @@ static efi_status_t EFIAPI efi_install_protocol_interface(void **handle,
>>
>>         /* Create new handle if requested. */
>>         if (!*handle) {
>> -               r = EFI_OUT_OF_RESOURCES;
>> -               goto out;
>> +               r = efi_create_handle(handle);
>> +               if (r != EFI_SUCCESS)
>> +                       goto out;
>>         }
>>
>>         /* Find object. */
>> --
>> 2.14.1
>>

[U-Boot] [PATCH 16/23] efi_loader: implement DisconnectController

[Heinrich Schuchardt]

On 08/31/2017 02:52 PM, Simon Glass wrote:
> On 27 August 2017 at 06:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  lib/efi_loader/efi_boottime.c | 77 ++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 76 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index 1069da7d79..c5a17b6252 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -1052,9 +1052,84 @@ static efi_status_t EFIAPI efi_disconnect_controller(void *controller_handle,
>>                                                      void *driver_image_handle,
>>                                                      void *child_handle)
> 
> Can you add a function comment?

Hello Simon,

the API functions (here DisconnectController) are documented in the UEFI
spec. Is your idea that we should duplicate part of this information for
all API functions? Or do you miss a comment on implementation details?

> 
>>  {
>> +       struct efi_driver_binding_protocol *binding_protocol;
>> +       efi_handle_t child_handle_buffer;
>> +       unsigned long driver_count;
>> +       efi_handle_t *driver_handle_buffer;
>> +       size_t i;
>> +       UINTN number_of_children;
> 
> Can we somehow use a lower-case type for this? Otherwise U-Boot will
> start to look like EFI and I will have to start taking
> antidepressants.
> 

In different places the EFI API requires a bitness dependent unsigned
integer.

Shall we globally rename UINTN to uintn?
This is my patch to blame:
503f2695548 (efi_loader: correct size for tpl level)

Regards

Heinrich

>> +       efi_status_t r;
>> +       size_t stop_count = 0;
>> +
>>         EFI_ENTRY("%p, %p, %p", controller_handle, driver_image_handle,
>>                   child_handle);
>> -       return EFI_EXIT(EFI_INVALID_PARAMETER);
>> +
>> +       if (!efi_search_obj(controller_handle)) {
>> +               r = EFI_INVALID_PARAMETER;
>> +               goto out;
>> +       }
>> +
>> +       /* Create list of driver handles */
>> +       if (driver_image_handle) {
>> +               driver_handle_buffer = &driver_image_handle,
>> +               driver_count = 1;
>> +               /* Check that the handle supports driver binding protocol */
>> +               r = efi_search_protocol(driver_image_handle,
>> +                                       &efi_guid_driver_binding_protocol,
>> +                                       NULL);
>> +       } else {
>> +               /* Get buffer with all handles with driver binding protocol */
>> +               r = EFI_CALL(efi_locate_handle_buffer(
>> +                            by_protocol, &efi_guid_driver_binding_protocol,
>> +                            NULL, &driver_count, &driver_handle_buffer));
>> +       }
>> +       if (r != EFI_SUCCESS)
>> +               goto out;
>> +
>> +       /* Create list of child handles */
>> +       if (child_handle) {
>> +               number_of_children = 1;
>> +               child_handle_buffer = &child_handle;
>> +       } else {
>> +               /*
>> +                * We do not fully support child handles.
>> +                *
>> +                * It is unclear from which handle and which protocols the
>> +                * list of child controllers should be collected.
>> +                */
>> +               number_of_children = 0;
>> +               child_handle_buffer = NULL;
>> +       }
>> +
>> +       for (i = 0; i < driver_count; ++i) {
>> +               r = EFI_CALL(efi_open_protocol(
>> +                            driver_handle_buffer[i],
>> +                            &efi_guid_driver_binding_protocol,
>> +                            (void **)&binding_protocol,
>> +                            driver_handle_buffer[i], NULL,
>> +                            EFI_OPEN_PROTOCOL_GET_PROTOCOL));
>> +               if (r != EFI_SUCCESS)
>> +                       continue;
>> +
>> +               r = EFI_CALL(binding_protocol->stop(binding_protocol,
>> +                                                   controller_handle,
>> +                                                   number_of_children,
>> +                                                   child_handle_buffer));
>> +               if (r == EFI_SUCCESS)
>> +                       ++stop_count;
>> +               EFI_CALL(efi_close_protocol(driver_handle_buffer[i],
>> +                                           &efi_guid_driver_binding_protocol,
>> +                                           driver_handle_buffer[i], NULL));
>> +       }
>> +
>> +       if (driver_image_handle)
>> +               efi_free_pool(driver_handle_buffer);
>> +       if (stop_count)
>> +               r = EFI_SUCCESS;
>> +       else
>> +               r = EFI_NOT_FOUND;
>> +out:
>> +       return EFI_EXIT(r);
>>  }
>>
>>  efi_status_t EFIAPI efi_close_protocol(void *handle, const efi_guid_t *protocol,
>> --
>> 2.14.1
>>
> 

[U-Boot] [PATCH 15/23] efi_loader: implement ConnectController

[Heinrich Schuchardt]

On 08/31/2017 02:52 PM, Simon Glass wrote:
> On 27 August 2017 at 06:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  include/efi_api.h             |  22 ++++++++
>>  include/efi_loader.h          |   1 +
>>  lib/efi_loader/efi_boottime.c | 119 +++++++++++++++++++++++++++++++++++++++++-
>>  3 files changed, 141 insertions(+), 1 deletion(-)
>>
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Please see below.
> 
>> diff --git a/include/efi_api.h b/include/efi_api.h
>> index 8efc8dfab8..b2838125d7 100644
>> --- a/include/efi_api.h
>> +++ b/include/efi_api.h
>> @@ -609,4 +609,26 @@ struct efi_pxe {
>>         struct efi_pxe_mode *mode;
>>  };
>>
>> +#define EFI_DRIVER_BINDING_PROTOCOL_GUID \
>> +       EFI_GUID(0x18a031ab, 0xb443, 0x4d1a,\
>> +                0xa5, 0xc0, 0x0c, 0x09, 0x26, 0x1e, 0x9f, 0x71)
>> +struct efi_driver_binding_protocol {
>> +       efi_status_t (EFIAPI * supported)(
> 
> I think the * should be up against 'supported'. Similarly below.

This is what checkpatch wants to see. Your suggestion leads to

ERROR: need consistent spacing around '*' (ctx:WxV)
#25: FILE: include/efi_api.h:616:
+       efi_status_t (EFIAPI *supported)(

So I prefer not change this before checkpatch.pl is not giving a
different result.

> 
>> +                       struct efi_driver_binding_protocol *this,
>> +                       efi_handle_t controller_handle,
>> +                       struct efi_device_path *remaining_device_path);
>> +       efi_status_t (EFIAPI * start)(
>> +                       struct efi_driver_binding_protocol *this,
>> +                       efi_handle_t controller_handle,
>> +                       struct efi_device_path *remaining_device_path);
>> +       efi_status_t (EFIAPI * stop)(
>> +                       struct efi_driver_binding_protocol *this,
>> +                       efi_handle_t controller_handle,
>> +                       UINTN number_of_children,
>> +                       efi_handle_t child_handle_buffer);
> 
> These should have function comments.
> 
>> +       u32 version;
>> +       efi_handle_t image_handle;
>> +       efi_handle_t driver_binding_handle;
>> +};
>> +
>>  #endif
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 9c68246c7c..f9f33e1d01 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -74,6 +74,7 @@ extern const struct efi_device_path_to_text_protocol efi_device_path_to_text;
>>
>>  extern const efi_guid_t efi_guid_console_control;
>>  extern const efi_guid_t efi_guid_device_path;
>> +extern const efi_guid_t efi_guid_driver_binding_protocol;
> 
> comment for this?

The GUIDs are defined in the UEFI spec.
So maybe we should just put a comment above all of these.

Regards

Heinrich

> 
>>  extern const efi_guid_t efi_guid_loaded_image;
>>  extern const efi_guid_t efi_guid_device_path_to_text_protocol;
>>
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index 5a73ea5cd0..1069da7d79 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -18,6 +18,14 @@
>>
>>  DECLARE_GLOBAL_DATA_PTR;
>>
>> +static efi_status_t EFIAPI efi_locate_protocol(const efi_guid_t *protocol,
>> +                                              void *registration,
>> +                                              void **protocol_interface);
>> +static efi_status_t EFIAPI efi_locate_handle_buffer(
>> +                       enum efi_locate_search_type search_type,
>> +                       const efi_guid_t *protocol, void *search_key,
>> +                       unsigned long *no_handles, efi_handle_t **buffer);
>> +
> 
> Is this a forward decl? Can you reorder (in a separate patch) to avoid it?
> 

[U-Boot] [PATCH 1/1] efi_loader: provide comment for protocol GUIDs

[Heinrich Schuchardt]

Add a missing comment.

Reported-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
Patch is directly applicable to efi-next.

Reported by Simon
Re: [PATCH 15/23] efi_loader: implement ConnectController
https://lists.denx.de/pipermail/u-boot/2017-August/304372.html
---
 include/efi_loader.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 46d684f6df..bc48bf4e11 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -67,6 +67,9 @@ extern struct efi_simple_input_interface efi_con_in;
 extern const struct efi_console_control_protocol efi_console_control;
 extern const struct efi_device_path_to_text_protocol efi_device_path_to_text;
 
+/*
+ * GUIDs of EFI protocols
+ */
 extern const efi_guid_t efi_guid_console_control;
 extern const efi_guid_t efi_guid_device_path;
 extern const efi_guid_t efi_guid_loaded_image;
-- 
2.11.0

[U-Boot] [PATCH 16/23] efi_loader: implement DisconnectController

[Simon Glass]

Hi Heinrich,

On 15 September 2017 at 00:35, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 08/31/2017 02:52 PM, Simon Glass wrote:
>> On 27 August 2017 at 06:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> ---
>>>  lib/efi_loader/efi_boottime.c | 77 ++++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 76 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>>> index 1069da7d79..c5a17b6252 100644
>>> --- a/lib/efi_loader/efi_boottime.c
>>> +++ b/lib/efi_loader/efi_boottime.c
>>> @@ -1052,9 +1052,84 @@ static efi_status_t EFIAPI efi_disconnect_controller(void *controller_handle,
>>>                                                      void *driver_image_handle,
>>>                                                      void *child_handle)
>>
>> Can you add a function comment?
>
> Hello Simon,
>
> the API functions (here DisconnectController) are documented in the UEFI
> spec. Is your idea that we should duplicate part of this information for
> all API functions? Or do you miss a comment on implementation details?

I think the code in U-Boot should stand alone, so arguments should be
described here along with a one-line function description. The args
are all void which is not good, but makes it even more important to
document.

>
>>
>>>  {
>>> +       struct efi_driver_binding_protocol *binding_protocol;
>>> +       efi_handle_t child_handle_buffer;
>>> +       unsigned long driver_count;
>>> +       efi_handle_t *driver_handle_buffer;
>>> +       size_t i;
>>> +       UINTN number_of_children;
>>
>> Can we somehow use a lower-case type for this? Otherwise U-Boot will
>> start to look like EFI and I will have to start taking
>> antidepressants.
>>
>
> In different places the EFI API requires a bitness dependent unsigned
> integer.
>
> Shall we globally rename UINTN to uintn?
> This is my patch to blame:
> 503f2695548 (efi_loader: correct size for tpl level)

What does bitness-dependent mean? Do you mean 32-bit?  It looks like
this is just passed to a function, so could be uint or instead?

If it is 32-bit then uint32_t should do.

Regards,
Simon

[U-Boot] [PATCH 15/23] efi_loader: implement ConnectController

[Simon Glass]

Hi Heinrich,

On 15 September 2017 at 00:48, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 08/31/2017 02:52 PM, Simon Glass wrote:
>> On 27 August 2017 at 06:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> ---
>>>  include/efi_api.h             |  22 ++++++++
>>>  include/efi_loader.h          |   1 +
>>>  lib/efi_loader/efi_boottime.c | 119 +++++++++++++++++++++++++++++++++++++++++-
>>>  3 files changed, 141 insertions(+), 1 deletion(-)
>>>
>>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>> Please see below.
>>
>>> diff --git a/include/efi_api.h b/include/efi_api.h
>>> index 8efc8dfab8..b2838125d7 100644
>>> --- a/include/efi_api.h
>>> +++ b/include/efi_api.h
>>> @@ -609,4 +609,26 @@ struct efi_pxe {
>>>         struct efi_pxe_mode *mode;
>>>  };
>>>
>>> +#define EFI_DRIVER_BINDING_PROTOCOL_GUID \
>>> +       EFI_GUID(0x18a031ab, 0xb443, 0x4d1a,\
>>> +                0xa5, 0xc0, 0x0c, 0x09, 0x26, 0x1e, 0x9f, 0x71)
>>> +struct efi_driver_binding_protocol {
>>> +       efi_status_t (EFIAPI * supported)(
>>
>> I think the * should be up against 'supported'. Similarly below.
>
> This is what checkpatch wants to see. Your suggestion leads to
>
> ERROR: need consistent spacing around '*' (ctx:WxV)
> #25: FILE: include/efi_api.h:616:
> +       efi_status_t (EFIAPI *supported)(
>
> So I prefer not change this before checkpatch.pl is not giving a
> different result.

OK I see. I suppose the EFIAPI is confusing it.

>
>>
>>> +                       struct efi_driver_binding_protocol *this,
>>> +                       efi_handle_t controller_handle,
>>> +                       struct efi_device_path *remaining_device_path);
>>> +       efi_status_t (EFIAPI * start)(
>>> +                       struct efi_driver_binding_protocol *this,
>>> +                       efi_handle_t controller_handle,
>>> +                       struct efi_device_path *remaining_device_path);
>>> +       efi_status_t (EFIAPI * stop)(
>>> +                       struct efi_driver_binding_protocol *this,
>>> +                       efi_handle_t controller_handle,
>>> +                       UINTN number_of_children,
>>> +                       efi_handle_t child_handle_buffer);
>>
>> These should have function comments.
>>
>>> +       u32 version;
>>> +       efi_handle_t image_handle;
>>> +       efi_handle_t driver_binding_handle;
>>> +};
>>> +
>>>  #endif
>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>> index 9c68246c7c..f9f33e1d01 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -74,6 +74,7 @@ extern const struct efi_device_path_to_text_protocol efi_device_path_to_text;
>>>
>>>  extern const efi_guid_t efi_guid_console_control;
>>>  extern const efi_guid_t efi_guid_device_path;
>>> +extern const efi_guid_t efi_guid_driver_binding_protocol;
>>
>> comment for this?
>
> The GUIDs are defined in the UEFI spec.
> So maybe we should just put a comment above all of these.

I know what a GUID is (or can look up efi_guid_t to find out) but not
what these variables are for.

Regards,
Simon

[U-Boot] [PATCH 16/23] efi_loader: implement DisconnectController

[Rob Clark]

On Wed, Sep 20, 2017 at 9:49 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Heinrich,
>
> On 15 September 2017 at 00:35, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> On 08/31/2017 02:52 PM, Simon Glass wrote:
>>> On 27 August 2017 at 06:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>> ---
>>>>  lib/efi_loader/efi_boottime.c | 77 ++++++++++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 76 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>>>> index 1069da7d79..c5a17b6252 100644
>>>> --- a/lib/efi_loader/efi_boottime.c
>>>> +++ b/lib/efi_loader/efi_boottime.c
>>>> @@ -1052,9 +1052,84 @@ static efi_status_t EFIAPI efi_disconnect_controller(void *controller_handle,
>>>>                                                      void *driver_image_handle,
>>>>                                                      void *child_handle)
>>>
>>> Can you add a function comment?
>>
>> Hello Simon,
>>
>> the API functions (here DisconnectController) are documented in the UEFI
>> spec. Is your idea that we should duplicate part of this information for
>> all API functions? Or do you miss a comment on implementation details?
>
> I think the code in U-Boot should stand alone, so arguments should be
> described here along with a one-line function description. The args
> are all void which is not good, but makes it even more important to
> document.

couple notes, fwiw..

1) As someone else implementing parts of UEFI interface, I'd find link
to section in spec (or perhaps to http://wiki.phoenix.com/) to be more
useful than re-writing the spec in our own words (and possibly getting
it wrong)

2) Leif introduced (iirc, in the stub HII or unicode patch)
efi_handle_t, and efi_string_t (and maybe we should add efi_char_t)..
which we should probably use more extensively.  Although I haven't
wanted to go on a major housecleaning while we still have so many
patches pending on list..  but would be a nice cleanup to make at some
point.

BR,
-R


>>
>>>
>>>>  {
>>>> +       struct efi_driver_binding_protocol *binding_protocol;
>>>> +       efi_handle_t child_handle_buffer;
>>>> +       unsigned long driver_count;
>>>> +       efi_handle_t *driver_handle_buffer;
>>>> +       size_t i;
>>>> +       UINTN number_of_children;
>>>
>>> Can we somehow use a lower-case type for this? Otherwise U-Boot will
>>> start to look like EFI and I will have to start taking
>>> antidepressants.
>>>
>>
>> In different places the EFI API requires a bitness dependent unsigned
>> integer.
>>
>> Shall we globally rename UINTN to uintn?
>> This is my patch to blame:
>> 503f2695548 (efi_loader: correct size for tpl level)
>
> What does bitness-dependent mean? Do you mean 32-bit?  It looks like
> this is just passed to a function, so could be uint or instead?
>
> If it is 32-bit then uint32_t should do.
>
> Regards,
> Simon

[U-Boot] [PATCH 16/23] efi_loader: implement DisconnectController

[Simon Glass]

Hi Rob,

On 20 September 2017 at 08:23, Rob Clark <robdclark@gmail.com> wrote:
> On Wed, Sep 20, 2017 at 9:49 AM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Heinrich,
>>
>> On 15 September 2017 at 00:35, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>> On 08/31/2017 02:52 PM, Simon Glass wrote:
>>>> On 27 August 2017 at 06:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>> ---
>>>>>  lib/efi_loader/efi_boottime.c | 77 ++++++++++++++++++++++++++++++++++++++++++-
>>>>>  1 file changed, 76 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>>>>> index 1069da7d79..c5a17b6252 100644
>>>>> --- a/lib/efi_loader/efi_boottime.c
>>>>> +++ b/lib/efi_loader/efi_boottime.c
>>>>> @@ -1052,9 +1052,84 @@ static efi_status_t EFIAPI efi_disconnect_controller(void *controller_handle,
>>>>>                                                      void *driver_image_handle,
>>>>>                                                      void *child_handle)
>>>>
>>>> Can you add a function comment?
>>>
>>> Hello Simon,
>>>
>>> the API functions (here DisconnectController) are documented in the UEFI
>>> spec. Is your idea that we should duplicate part of this information for
>>> all API functions? Or do you miss a comment on implementation details?
>>
>> I think the code in U-Boot should stand alone, so arguments should be
>> described here along with a one-line function description. The args
>> are all void which is not good, but makes it even more important to
>> document.
>
> couple notes, fwiw..
>
> 1) As someone else implementing parts of UEFI interface, I'd find link
> to section in spec (or perhaps to http://wiki.phoenix.com/) to be more
> useful than re-writing the spec in our own words (and possibly getting
> it wrong)

The problem is that there are 3 void pointers and I have no idea what
they really are and what to pass. We have to have some coding
standards in U-Boot. I am not looking for a detailed description of
the purpose of the function, but one line and a description of the
arguments is the minimum we should have for exported functions.

I think it is a great idea to link to the spec as well, particularly
if it can be a URL.

>
> 2) Leif introduced (iirc, in the stub HII or unicode patch)
> efi_handle_t, and efi_string_t (and maybe we should add efi_char_t)..
> which we should probably use more extensively.  Although I haven't
> wanted to go on a major housecleaning while we still have so many
> patches pending on list..  but would be a nice cleanup to make at some
> point.

Sounds good to me. No hurry, but it's nice to know that this is
heading in the right direction. The EFI API is really awful IMO. The
obfuscation is so painful.

Regards,
Simon

[U-Boot] [PATCH 1/1] efi_loader: provide comment for protocol GUIDs

[Simon Glass]

On 15 September 2017 at 01:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Add a missing comment.
>
> Reported-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> Patch is directly applicable to efi-next.
>
> Reported by Simon
> Re: [PATCH 15/23] efi_loader: implement ConnectController
> https://lists.denx.de/pipermail/u-boot/2017-August/304372.html
> ---
>  include/efi_loader.h | 3 +++
>  1 file changed, 3 insertions(+)

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