[U-Boot] [PATCH] efi_loader: add back optional efi_handler::open()

[U-Boot] [PATCH] efi_loader: add back optional efi_handler::open()

[Rob Clark]

In some cases it is useful to defer creation of the protocol interface
object.  So add back an optional ->open() hook that is used if
protcol_interface is NULL.

I've slightly simplified the fxn ptr signature to remove unneeded args,
and so compiler will complain if patches that used the "old way" are,
and which do not need this extra complexity, are rebased.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 include/efi_loader.h          | 9 ++++++++-
 lib/efi_loader/efi_boottime.c | 7 +++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 90411cdbab..3da0477fdc 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -41,10 +41,17 @@ extern unsigned int __efi_runtime_rel_start, __efi_runtime_rel_stop;
 /*
  * When the UEFI payload wants to open a protocol on an object to get its
  * interface (usually a struct with callback functions), this struct maps the
- * protocol GUID to the respective protocol interface */
+ * protocol GUID to the respective protocol interface.
+ *
+ * The optional ->open() fxn can be used for cases where the protocol
+ * interface is constructed on-demand, and is called if protocol_interface
+ * is NULL.
+ */
 struct efi_handler {
 	const efi_guid_t *guid;
 	void *protocol_interface;
+	efi_status_t (EFIAPI *open)(void *handle, efi_guid_t *protocol,
+			void **protocol_interface);
 };
 
 /*
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 7d45c18ff7..21f41866d0 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1063,6 +1063,13 @@ static efi_status_t EFIAPI efi_open_protocol(
 			if (!guidcmp(hprotocol, protocol)) {
 				if (attributes !=
 				    EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
+					if (!handler->protocol_interface) {
+						r = handler->open(efiobj->handle,
+								protocol,
+								&handler->protocol_interface);
+						if (r != EFI_SUCCESS)
+							goto out;
+					}
 					*protocol_interface =
 						handler->protocol_interface;
 				}
-- 
2.13.0

[U-Boot] [PATCH 1/2] efi_loader: add helper macro to construct protocol objects

[Rob Clark]

There are a bunch of protocols which should be exposed by GUID but are
not.  Add a helper macro to create an efi_object, to avoid much typing.

Note that using the pointer for efiobj->handle is semi-arbitrary.  We
just need a unique value to match the efiobj supporting the protocol
with the handle that LocateHandle() returns..

See LibLocateProtocol() in gnu-efi.  It does LocateHandle() to find all
the handles, and then loops over them calling HandleProtocol() with the
GUID of the protocol it is trying to find.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 include/efi_loader.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 27c741ba0c..51fbf23864 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -63,6 +63,14 @@ struct efi_object {
 	void *handle;
 };
 
+#define EFI_PROTOCOL_OBJECT(_guid, _protocol) (struct efi_object){	\
+	.protocols = {{							\
+		.guid = &(_guid),	 				\
+		.protocol_interface = (void *)(_protocol), 		\
+	}},								\
+	.handle = (void *)(_protocol),					\
+}
+
 /**
  * struct efi_event
  *
-- 
2.13.0

[U-Boot] [PATCH 2/2] efi_loader: expose protocols via GUID

[Rob Clark]

shim.efi (or rather gnu-efi's LibLocateProtocol() which shim.efi uses)
resolves protocols via efi_locate_handle() so the console protocols
need to be added to the efi object list.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 include/efi_api.h            |  9 +++++++++
 include/efi_loader.h         |  1 +
 lib/efi_loader/efi_console.c | 15 ++++++++++++++-
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/include/efi_api.h b/include/efi_api.h
index 2c443080b6..d8fecb6980 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -393,6 +393,11 @@ struct simple_text_output_mode {
 	bool cursor_visible;
 };
 
+
+#define EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL_GUID \
+	EFI_GUID(0x387477c2, 0x69c7, 0x11d2, \
+		 0x8e, 0x39, 0x0, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
+
 struct efi_simple_text_output_protocol {
 	void *reset;
 	efi_status_t (EFIAPI *output_string)(
@@ -427,6 +432,10 @@ struct efi_input_key {
 	s16 unicode_char;
 };
 
+#define EFI_SIMPLE_TEXT_INPUT_PROTOCOL_GUID \
+	EFI_GUID(0x387477c1, 0x69c7, 0x11d2, \
+		 0x8e, 0x39, 0x0, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
+
 struct efi_simple_input_interface {
 	efi_status_t(EFIAPI *reset)(struct efi_simple_input_interface *this,
 			bool ExtendedVerification);
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 51fbf23864..48eeb4cdd8 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -109,6 +109,7 @@ int efi_net_register(void **handle);
 /* 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);
 
diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
index dbe98ac08b..ba0a8b90cd 100644
--- a/lib/efi_loader/efi_console.c
+++ b/lib/efi_loader/efi_console.c
@@ -426,7 +426,6 @@ struct efi_simple_input_interface efi_con_in = {
 	.read_key_stroke = efi_cin_read_key_stroke,
 	.wait_for_key = NULL,
 };
-
 static struct efi_event *console_timer_event;
 
 static void efi_key_notify(struct efi_event *event, void *context)
@@ -441,10 +440,24 @@ static void efi_console_timer_notify(struct efi_event *event, void *context)
 	EFI_EXIT(EFI_SUCCESS);
 }
 
+
+static struct efi_object efi_console_control_obj =
+	EFI_PROTOCOL_OBJECT(efi_guid_console_control, &efi_console_control);
+static struct efi_object efi_console_output_obj =
+	EFI_PROTOCOL_OBJECT(EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL_GUID, &efi_con_out);
+static struct efi_object efi_console_input_obj =
+	EFI_PROTOCOL_OBJECT(EFI_SIMPLE_TEXT_INPUT_PROTOCOL_GUID, &efi_con_in);
+
 /* This gets called from do_bootefi_exec(). */
 int efi_console_register(void)
 {
 	efi_status_t r;
+
+	/* Hook up to the device list */
+	list_add_tail(&efi_console_control_obj.link, &efi_obj_list);
+	list_add_tail(&efi_console_output_obj.link, &efi_obj_list);
+	list_add_tail(&efi_console_input_obj.link, &efi_obj_list);
+
 	r = efi_create_event(EVT_NOTIFY_WAIT, TPL_CALLBACK,
 			     efi_key_notify, NULL, &efi_con_in.wait_for_key);
 	if (r != EFI_SUCCESS) {
-- 
2.13.0

[U-Boot] [PATCH] efi_loader: add back optional efi_handler::open()

[Alexander Graf]

On 24.07.17 16:38, Rob Clark wrote:
> In some cases it is useful to defer creation of the protocol interface
> object.  So add back an optional ->open() hook that is used if
> protcol_interface is NULL.
> 
> I've slightly simplified the fxn ptr signature to remove unneeded args,
> and so compiler will complain if patches that used the "old way" are,
> and which do not need this extra complexity, are rebased.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>   include/efi_loader.h          | 9 ++++++++-
>   lib/efi_loader/efi_boottime.c | 7 +++++++
>   2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 90411cdbab..3da0477fdc 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -41,10 +41,17 @@ extern unsigned int __efi_runtime_rel_start, __efi_runtime_rel_stop;
>   /*
>    * When the UEFI payload wants to open a protocol on an object to get its
>    * interface (usually a struct with callback functions), this struct maps the
> - * protocol GUID to the respective protocol interface */
> + * protocol GUID to the respective protocol interface.
> + *
> + * The optional ->open() fxn can be used for cases where the protocol
> + * interface is constructed on-demand, and is called if protocol_interface
> + * is NULL.
> + */
>   struct efi_handler {
>   	const efi_guid_t *guid;
>   	void *protocol_interface;
> +	efi_status_t (EFIAPI *open)(void *handle, efi_guid_t *protocol,
> +			void **protocol_interface);
>   };
>   
>   /*
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 7d45c18ff7..21f41866d0 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1063,6 +1063,13 @@ static efi_status_t EFIAPI efi_open_protocol(
>   			if (!guidcmp(hprotocol, protocol)) {
>   				if (attributes !=
>   				    EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
> +					if (!handler->protocol_interface) {
> +						r = handler->open(efiobj->handle,
> +								protocol,
> +								&handler->protocol_interface);

With this patch you basically allow deferred setting of 
handler->protocol_interface. I think that's a reasonable thing to do, 
but we have to be careful not to break UEFI assumptions.

For example efi_locate_protocol reads handler->protocol_interface, so 
that needs adjustment I suppose.


Alex

[U-Boot] [PATCH] efi_loader: add back optional efi_handler::open()

[Rob Clark]

In some cases it is useful to defer creation of the protocol interface
object.  So add back an optional ->open() hook that is used if
protcol_interface is NULL.

I've slightly simplified the fxn ptr signature to remove unneeded args,
and so compiler will complain if patches that used the "old way" are,
and which do not need this extra complexity, are rebased.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
v2: also handle this properly in locate_protocol as agraf pointed out

Note that I might be misunderstanding what the EFI application expects
when looking up device-path on a file handle.. if this is only expected
to be the device path up to the partition object (and not including the
file-path part of the path), then maybe ->open() isn't *as* important
as I thought it would be.  OTOH I think it would be cleaner to use for
loaded_image_info_obj and bootefi_device_obj.. currently I have to
patch in the real boot device-path in efi_set_bootdev(), which is
messy and fragile.

I don't fully understand Heinrich's concerns, especially since the
EFI_OPEN_PROTOCOL_INFORMATION_ENTRY stuff looks like it only cares
about already opened protocols, which should already have their open()
called.  (And either way, since open() is optional maybe it is enough
to just not use it for any efiobj's where it would be problematic?)

So tl;dr, if this is really going to be a problem for some things that
Heinrich has got in the pipeline, maybe (pending clarification of a
file handle's device-path) we can get by without this.  But if it is
not a problem, then it certainly seems useful in a few places.

 include/efi_loader.h          |  9 ++++++++-
 lib/efi_loader/efi_boottime.c | 30 ++++++++++++++++++++++++------
 2 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 48eeb4cdd8..a9888afb04 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -41,10 +41,17 @@ extern unsigned int __efi_runtime_rel_start, __efi_runtime_rel_stop;
 /*
  * When the UEFI payload wants to open a protocol on an object to get its
  * interface (usually a struct with callback functions), this struct maps the
- * protocol GUID to the respective protocol interface */
+ * protocol GUID to the respective protocol interface.
+ *
+ * The optional ->open() fxn can be used for cases where the protocol
+ * interface is constructed on-demand, and is called if protocol_interface
+ * is NULL.
+ */
 struct efi_handler {
 	const efi_guid_t *guid;
 	void *protocol_interface;
+	efi_status_t (EFIAPI *open)(void *handle, const efi_guid_t *protocol,
+			void **protocol_interface);
 };
 
 /*
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 7d45c18ff7..80d9024a53 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -903,6 +903,23 @@ out:
 	return EFI_EXIT(r);
 }
 
+static efi_status_t open_protocol(struct efi_object *efiobj,
+				  struct efi_handler *handler,
+				  void **protocol_interface)
+{
+	efi_status_t ret = EFI_SUCCESS;
+
+	if (!handler->protocol_interface) {
+		ret = handler->open(efiobj->handle,
+				handler->guid,
+				&handler->protocol_interface);
+	}
+	*protocol_interface =
+		handler->protocol_interface;
+
+	return ret;
+}
+
 static efi_status_t EFIAPI efi_locate_protocol(efi_guid_t *protocol,
 					       void *registration,
 					       void **protocol_interface)
@@ -925,9 +942,10 @@ static efi_status_t EFIAPI efi_locate_protocol(efi_guid_t *protocol,
 			if (!handler->guid)
 				continue;
 			if (!guidcmp(handler->guid, protocol)) {
-				*protocol_interface =
-					handler->protocol_interface;
-				return EFI_EXIT(EFI_SUCCESS);
+				efi_status_t ret;
+				ret = open_protocol(efiobj, handler,
+						    protocol_interface);
+				return EFI_EXIT(ret);
 			}
 		}
 	}
@@ -1061,12 +1079,12 @@ static efi_status_t EFIAPI efi_open_protocol(
 			if (!hprotocol)
 				continue;
 			if (!guidcmp(hprotocol, protocol)) {
+				r = EFI_SUCCESS;
 				if (attributes !=
 				    EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
-					*protocol_interface =
-						handler->protocol_interface;
+					r = open_protocol(efiobj, handler,
+							  protocol_interface);
 				}
-				r = EFI_SUCCESS;
 				goto out;
 			}
 		}
-- 
2.13.0

[U-Boot] [PATCH] efi_loader: add back optional efi_handler::open()

[Heinrich Schuchardt]

On 07/25/2017 02:21 PM, Rob Clark wrote:
> In some cases it is useful to defer creation of the protocol interface
> object.  So add back an optional ->open() hook that is used if
> protcol_interface is NULL.
> 
> I've slightly simplified the fxn ptr signature to remove unneeded args,
> and so compiler will complain if patches that used the "old way" are,
> and which do not need this extra complexity, are rebased.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
> v2: also handle this properly in locate_protocol as agraf pointed out
> 
> Note that I might be misunderstanding what the EFI application expects
> when looking up device-path on a file handle.. if this is only expected
> to be the device path up to the partition object (and not including the
> file-path part of the path), then maybe ->open() isn't *as* important
> as I thought it would be.  OTOH I think it would be cleaner to use for
> loaded_image_info_obj and bootefi_device_obj.. currently I have to
> patch in the real boot device-path in efi_set_bootdev(), which is
> messy and fragile.
> 
> I don't fully understand Heinrich's concerns, especially since the
> EFI_OPEN_PROTOCOL_INFORMATION_ENTRY stuff looks like it only cares
> about already opened protocols, which should already have their open()
> called.  (And either way, since open() is optional maybe it is enough
> to just not use it for any efiobj's where it would be problematic?)
> 
> So tl;dr, if this is really going to be a problem for some things that
> Heinrich has got in the pipeline, maybe (pending clarification of a
> file handle's device-path) we can get by without this.  But if it is
> not a problem, then it certainly seems useful in a few places.
> 
>  include/efi_loader.h          |  9 ++++++++-
>  lib/efi_loader/efi_boottime.c | 30 ++++++++++++++++++++++++------
>  2 files changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 48eeb4cdd8..a9888afb04 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -41,10 +41,17 @@ extern unsigned int __efi_runtime_rel_start, __efi_runtime_rel_stop;
>  /*
>   * When the UEFI payload wants to open a protocol on an object to get its
>   * interface (usually a struct with callback functions), this struct maps the
> - * protocol GUID to the respective protocol interface */
> + * protocol GUID to the respective protocol interface.
> + *
> + * The optional ->open() fxn can be used for cases where the protocol
> + * interface is constructed on-demand, and is called if protocol_interface
> + * is NULL.
> + */
>  struct efi_handler {
>  	const efi_guid_t *guid;
>  	void *protocol_interface;
> +	efi_status_t (EFIAPI *open)(void *handle, const efi_guid_t *protocol,
> +			void **protocol_interface);
>  };
>  
>  /*
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 7d45c18ff7..80d9024a53 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -903,6 +903,23 @@ out:
>  	return EFI_EXIT(r);
>  }
>  
> +static efi_status_t open_protocol(struct efi_object *efiobj,
> +				  struct efi_handler *handler,
> +				  void **protocol_interface)
> +{
> +	efi_status_t ret = EFI_SUCCESS;
> +
> +	if (!handler->protocol_interface) {
> +		ret = handler->open(efiobj->handle,
> +				handler->guid,
> +				&handler->protocol_interface);
> +	}
> +	*protocol_interface =
> +		handler->protocol_interface;
> +
> +	return ret;
> +}
> +
>  static efi_status_t EFIAPI efi_locate_protocol(efi_guid_t *protocol,
>  					       void *registration,
>  					       void **protocol_interface)
> @@ -925,9 +942,10 @@ static efi_status_t EFIAPI efi_locate_protocol(efi_guid_t *protocol,
>  			if (!handler->guid)
>  				continue;
>  			if (!guidcmp(handler->guid, protocol)) {
> -				*protocol_interface =
> -					handler->protocol_interface;
> -				return EFI_EXIT(EFI_SUCCESS);
> +				efi_status_t ret;
> +				ret = open_protocol(efiobj, handler,
> +						    protocol_interface);

NAK

With your design a driver could decide that on the first call of
open_protocol it exposes a protocol and on the second it does not.

Or LocateHandleBuffer return an object meant to support a protocol and
then opening is refused.

The UEFI standard assumes that the supported protocols can be reliably
be determined once and for all.

If anything, please, use an init function of type:
void init(efi_guid guid);
Simply pass the protocol guid to the init function of the handler.

How many ms and kiB do you think you will save when starting grub with
deferred initialization?

Best regards

Heinrich


> +				return EFI_EXIT(ret);
>  			}
>  		}
>  	}
> @@ -1061,12 +1079,12 @@ static efi_status_t EFIAPI efi_open_protocol(
>  			if (!hprotocol)
>  				continue;
>  			if (!guidcmp(hprotocol, protocol)) {
> +				r = EFI_SUCCESS;
>  				if (attributes !=
>  				    EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
> -					*protocol_interface =
> -						handler->protocol_interface;
> +					r = open_protocol(efiobj, handler,
> +							  protocol_interface);
>  				}
> -				r = EFI_SUCCESS;
>  				goto out;
>  			}
>  		}
> 

[U-Boot] [PATCH] efi_loader: add back optional efi_handler::open()

[Rob Clark]

On Tue, Jul 25, 2017 at 12:39 PM, Heinrich Schuchardt
<xypron.glpk@gmx.de> wrote:
> On 07/25/2017 02:21 PM, Rob Clark wrote:
>> In some cases it is useful to defer creation of the protocol interface
>> object.  So add back an optional ->open() hook that is used if
>> protcol_interface is NULL.
>>
>> I've slightly simplified the fxn ptr signature to remove unneeded args,
>> and so compiler will complain if patches that used the "old way" are,
>> and which do not need this extra complexity, are rebased.
>>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> ---
>> v2: also handle this properly in locate_protocol as agraf pointed out
>>
>> Note that I might be misunderstanding what the EFI application expects
>> when looking up device-path on a file handle.. if this is only expected
>> to be the device path up to the partition object (and not including the
>> file-path part of the path), then maybe ->open() isn't *as* important
>> as I thought it would be.  OTOH I think it would be cleaner to use for
>> loaded_image_info_obj and bootefi_device_obj.. currently I have to
>> patch in the real boot device-path in efi_set_bootdev(), which is
>> messy and fragile.
>>
>> I don't fully understand Heinrich's concerns, especially since the
>> EFI_OPEN_PROTOCOL_INFORMATION_ENTRY stuff looks like it only cares
>> about already opened protocols, which should already have their open()
>> called.  (And either way, since open() is optional maybe it is enough
>> to just not use it for any efiobj's where it would be problematic?)
>>
>> So tl;dr, if this is really going to be a problem for some things that
>> Heinrich has got in the pipeline, maybe (pending clarification of a
>> file handle's device-path) we can get by without this.  But if it is
>> not a problem, then it certainly seems useful in a few places.
>>
>>  include/efi_loader.h          |  9 ++++++++-
>>  lib/efi_loader/efi_boottime.c | 30 ++++++++++++++++++++++++------
>>  2 files changed, 32 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 48eeb4cdd8..a9888afb04 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -41,10 +41,17 @@ extern unsigned int __efi_runtime_rel_start, __efi_runtime_rel_stop;
>>  /*
>>   * When the UEFI payload wants to open a protocol on an object to get its
>>   * interface (usually a struct with callback functions), this struct maps the
>> - * protocol GUID to the respective protocol interface */
>> + * protocol GUID to the respective protocol interface.
>> + *
>> + * The optional ->open() fxn can be used for cases where the protocol
>> + * interface is constructed on-demand, and is called if protocol_interface
>> + * is NULL.
>> + */
>>  struct efi_handler {
>>       const efi_guid_t *guid;
>>       void *protocol_interface;
>> +     efi_status_t (EFIAPI *open)(void *handle, const efi_guid_t *protocol,
>> +                     void **protocol_interface);
>>  };
>>
>>  /*
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index 7d45c18ff7..80d9024a53 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -903,6 +903,23 @@ out:
>>       return EFI_EXIT(r);
>>  }
>>
>> +static efi_status_t open_protocol(struct efi_object *efiobj,
>> +                               struct efi_handler *handler,
>> +                               void **protocol_interface)
>> +{
>> +     efi_status_t ret = EFI_SUCCESS;
>> +
>> +     if (!handler->protocol_interface) {
>> +             ret = handler->open(efiobj->handle,
>> +                             handler->guid,
>> +                             &handler->protocol_interface);
>> +     }
>> +     *protocol_interface =
>> +             handler->protocol_interface;
>> +
>> +     return ret;
>> +}
>> +
>>  static efi_status_t EFIAPI efi_locate_protocol(efi_guid_t *protocol,
>>                                              void *registration,
>>                                              void **protocol_interface)
>> @@ -925,9 +942,10 @@ static efi_status_t EFIAPI efi_locate_protocol(efi_guid_t *protocol,
>>                       if (!handler->guid)
>>                               continue;
>>                       if (!guidcmp(handler->guid, protocol)) {
>> -                             *protocol_interface =
>> -                                     handler->protocol_interface;
>> -                             return EFI_EXIT(EFI_SUCCESS);
>> +                             efi_status_t ret;
>> +                             ret = open_protocol(efiobj, handler,
>> +                                                 protocol_interface);
>
> NAK
>
> With your design a driver could decide that on the first call of
> open_protocol it exposes a protocol and on the second it does not.

Not true, please read the patch again.  ->open() is only called the first time.

> Or LocateHandleBuffer return an object meant to support a protocol and
> then opening is refused.
>
> The UEFI standard assumes that the supported protocols can be reliably
> be determined once and for all.
>
> If anything, please, use an init function of type:
> void init(efi_guid guid);
> Simply pass the protocol guid to the init function of the handler.
>
> How many ms and kiB do you think you will save when starting grub with
> deferred initialization?

I did confirm that the device-path for an file object should include
the file path.  So this also avoids needing to construct a device path
on each file open, which will most likely never be accessed.

BR,
-R

> Best regards
>
> Heinrich
>
>
>> +                             return EFI_EXIT(ret);
>>                       }
>>               }
>>       }
>> @@ -1061,12 +1079,12 @@ static efi_status_t EFIAPI efi_open_protocol(
>>                       if (!hprotocol)
>>                               continue;
>>                       if (!guidcmp(hprotocol, protocol)) {
>> +                             r = EFI_SUCCESS;
>>                               if (attributes !=
>>                                   EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
>> -                                     *protocol_interface =
>> -                                             handler->protocol_interface;
>> +                                     r = open_protocol(efiobj, handler,
>> +                                                       protocol_interface);
>>                               }
>> -                             r = EFI_SUCCESS;
>>                               goto out;
>>                       }
>>               }
>>
>

[U-Boot] [U-Boot,2/2] efi_loader: expose protocols via GUID

[Alexander Graf]

> shim.efi (or rather gnu-efi's LibLocateProtocol() which shim.efi uses)
> resolves protocols via efi_locate_handle() so the console protocols
> need to be added to the efi object list.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

Thanks, applied to efi-next

Alex

[U-Boot] [U-Boot, 1/2] efi_loader: add helper macro to construct protocol objects

[Alexander Graf]

> There are a bunch of protocols which should be exposed by GUID but are
> not.  Add a helper macro to create an efi_object, to avoid much typing.
> 
> Note that using the pointer for efiobj->handle is semi-arbitrary.  We
> just need a unique value to match the efiobj supporting the protocol
> with the handle that LocateHandle() returns..
> 
> See LibLocateProtocol() in gnu-efi.  It does LocateHandle() to find all
> the handles, and then loops over them calling HandleProtocol() with the
> GUID of the protocol it is trying to find.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

Thanks, applied to efi-next

Alex