[PATCH v3 0/3] efi_loader: OpenProtocol, CloseProtocol

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v3 0/3] efi_loader: OpenProtocol, CloseProtocol

Heinrich Schuchardt
The implementation of OpenProtocol was incomplete up to now.
CloseProtocol and OpenProtocolInformation were not implemented.

The first patch prints a debug message with the protocol GUID in
OpenProtocol.

The second patch adds a table of efi_open_protocol_info_entry to keep
track of locks to each protocol entry of each handle. This table is
maintained in OpenProtocol and CloseProtocol.

The third patch implements OpenProtocolInformation which returns said
table.
---
v3
        Use list_for_each_entry
        Fix indention
        Use EFI_CALL to avoid wrapper for efi_disconnect_controller
       
v2
        Fixed typo in 1st patch which is resubmitted.
        Added 2nd and 3rd patch.
---
Heinrich Schuchardt (3):
  efi_loader: write protocol GUID in OpenProtocol
  efi_loader: open_info in OpenProtocol, CloseProtocol
  efi_loader: implement OpenProtocolInformation

 include/efi_loader.h          |  15 +++
 lib/efi_loader/efi_boottime.c | 240 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 244 insertions(+), 11 deletions(-)

--
2.11.0

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v3 1/3] efi_loader: write protocol GUID in OpenProtocol

Heinrich Schuchardt
To understand what happens in OpenProtocol it is necessary to know
the protocol interface GUID. Let's write a debug message.

Using uuid_guid_get_str would be quite clumsy for this purpose.
This would involve evaluating _DEBUG which probably should not be used
outside common.h.

Cc: Rob Clark <[hidden email]>
Signed-off-by: Heinrich Schuchardt <[hidden email]>
---
v3:
        reference uuid_guid_get_str
v2:
        fix typo in commit message
---
 include/efi_loader.h          | 14 ++++++++++++++
 lib/efi_loader/efi_boottime.c |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 037cc7c543..553c615f11 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -51,6 +51,20 @@ const char *__efi_nesting_dec(void);
  debug("%sEFI: Return From: %s\n", __efi_nesting_dec(), #exp); \
  } while(0)
 
+/*
+ * Write GUID
+ */
+#define EFI_PRINT_GUID(txt, guid) ({ \
+ debug("EFI: %s %02x%02x%02x%02x-%02x%02x-%02x%02x-" \
+      "%02x%02x%02x%02x%02x%02x%02x%02x\n", \
+      txt, ((u8 *)guid)[3], \
+      ((u8 *)guid)[2], ((u8 *)guid)[1], ((u8 *)guid)[0], \
+      ((u8 *)guid)[5], ((u8 *)guid)[4], ((u8 *)guid)[7], \
+      ((u8 *)guid)[6], ((u8 *)guid)[8], ((u8 *)guid)[9], \
+      ((u8 *)guid)[10], ((u8 *)guid)[11], ((u8 *)guid)[12], \
+      ((u8 *)guid)[13], ((u8 *)guid)[14], ((u8 *)guid)[15]); \
+ })
+
 extern struct efi_runtime_services efi_runtime_services;
 extern struct efi_system_table systab;
 
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index fb05c9e093..ebb557abb2 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1138,6 +1138,8 @@ static efi_status_t EFIAPI efi_open_protocol(
  goto out;
  }
 
+ EFI_PRINT_GUID("protocol:", protocol);
+
  switch (attributes) {
  case EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL:
  case EFI_OPEN_PROTOCOL_GET_PROTOCOL:
--
2.11.0

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v3 2/3] efi_loader: open_info in OpenProtocol, CloseProtocol

Heinrich Schuchardt
In reply to this post by 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.

Cc: Rob Clark <[hidden email]>
Signed-off-by: Heinrich Schuchardt <[hidden email]>
---
v3:
        use EFI_CALL to avoid wrapper for efi_disconnect_controller
        use list_for_each_entry
        move variable declarations out of loops
v2:
        new patch
---
 include/efi_loader.h          |   1 +
 lib/efi_loader/efi_boottime.c | 164 +++++++++++++++++++++++++++++++++++++++---
 2 files changed, 155 insertions(+), 10 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 553c615f11..222b251a38 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -88,6 +88,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 ebb557abb2..e969814476 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -898,23 +898,78 @@ static efi_status_t EFIAPI efi_connect_controller(
  return EFI_EXIT(EFI_NOT_FOUND);
 }
 
-static efi_status_t EFIAPI efi_disconnect_controller(void *controller_handle,
-     void *driver_image_handle,
-     void *child_handle)
+static efi_status_t EFIAPI efi_disconnect_controller(
+ void *controller_handle,
+ void *driver_image_handle,
+ void *child_handle)
 {
  EFI_ENTRY("%p, %p, %p", controller_handle, driver_image_handle,
   child_handle);
  return EFI_EXIT(EFI_INVALID_PARAMETER);
 }
 
+static efi_status_t efi_close_protocol_int(struct efi_handler *protocol,
+   void *agent_handle,
+   void *controller_handle)
+{
+ size_t i;
+ struct efi_open_protocol_info_entry *open_info;
+
+ 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 &&
+    open_info->controller_handle ==
+    controller_handle) {
+ open_info->open_count--;
+ return EFI_SUCCESS;
+ }
+ }
+ return EFI_NOT_FOUND;
+}
+
 static efi_status_t EFIAPI efi_close_protocol(void *handle,
       efi_guid_t *protocol,
       void *agent_handle,
       void *controller_handle)
 {
+ struct efi_object *efiobj;
+ size_t i;
+ efi_status_t r = EFI_NOT_FOUND;
+
  EFI_ENTRY("%p, %p, %p, %p", handle, protocol, agent_handle,
   controller_handle);
- return EFI_EXIT(EFI_NOT_FOUND);
+
+ if (!handle || !protocol || !agent_handle) {
+ r = EFI_INVALID_PARAMETER;
+ goto out;
+ }
+
+ EFI_PRINT_GUID("protocol:", protocol);
+
+ list_for_each_entry(efiobj, &efi_obj_list, 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)) {
+ r = efi_close_protocol_int(handler,
+   agent_handle,
+   controller_handle);
+ goto out;
+ }
+ }
+ goto out;
+ }
+out:
+ return EFI_EXIT(r);
 }
 
 static efi_status_t EFIAPI efi_open_protocol_information(efi_handle_t handle,
@@ -1119,6 +1174,96 @@ static void EFIAPI efi_set_mem(void *buffer, unsigned long size, uint8_t value)
  memset(buffer, value, size);
 }
 
+static efi_status_t efi_open_protocol_int(
+ 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,
@@ -1173,12 +1318,11 @@ static efi_status_t EFIAPI efi_open_protocol(
  if (!hprotocol)
  continue;
  if (!guidcmp(hprotocol, protocol)) {
- if (attributes !=
-    EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
- *protocol_interface =
- handler->protocol_interface;
- }
- r = EFI_SUCCESS;
+ r = efi_open_protocol_int(handler,
+  protocol_interface,
+  agent_handle,
+  controller_handle,
+  attributes);
  goto out;
  }
  }
--
2.11.0

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v3 3/3] efi_loader: implement OpenProtocolInformation

Heinrich Schuchardt
In reply to this post by 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 <[hidden email]>
Signed-off-by: Heinrich Schuchardt <[hidden email]>
---
v3:
        use list_for_each_entry
        correct indention
v2:
        new patch
---
 lib/efi_loader/efi_boottime.c | 74 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 73 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 65a7a79dc1..fa9b74d465 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -984,14 +984,86 @@ out:
  return EFI_EXIT(r);
 }
 
+static efi_status_t efi_copy_open_protocol_information(
+ struct efi_handler *protocol,
+ struct efi_open_protocol_info_entry **entry_buffer,
+ unsigned long *entry_count)
+{
+ unsigned long buffer_size;
+ unsigned long count = 0;
+ size_t i;
+ efi_status_t r;
+
+ *entry_buffer = NULL;
+
+ /* Count entries */
+ for (i = 0; i < ARRAY_SIZE(protocol->open_info); ++i) {
+ struct efi_open_protocol_info_entry *open_info =
+ &protocol->open_info[i];
+
+ if (open_info->open_count)
+ ++count;
+ }
+ *entry_count = count;
+ if (!count)
+ return EFI_SUCCESS;
+
+ /* 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)
+ return r;
+ count = 0;
+ for (i = 0; i < ARRAY_SIZE(protocol->open_info); ++i) {
+ struct efi_open_protocol_info_entry *open_info =
+ &protocol->open_info[i];
+
+ if (!open_info->open_count)
+ continue;
+ (*entry_buffer)[count] = *open_info;
+ ++count;
+ }
+
+ return EFI_SUCCESS;
+}
+
 static efi_status_t EFIAPI efi_open_protocol_information(efi_handle_t handle,
  efi_guid_t *protocol,
  struct efi_open_protocol_info_entry **entry_buffer,
  unsigned long *entry_count)
 {
+ struct efi_object *efiobj;
+ size_t i;
+ efi_status_t r = EFI_NOT_FOUND;
+
  EFI_ENTRY("%p, %p, %p, %p", handle, protocol, entry_buffer,
   entry_count);
- return EFI_EXIT(EFI_NOT_FOUND);
+
+ if (!handle || !protocol || !entry_buffer)
+ EFI_EXIT(EFI_INVALID_PARAMETER);
+
+ EFI_PRINT_GUID("protocol:", protocol);
+
+ list_for_each_entry(efiobj, &efi_obj_list, 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)) {
+ r = efi_copy_open_protocol_information(
+ handler, entry_buffer, entry_count);
+ goto out;
+ }
+ }
+ goto out;
+ }
+out:
+ return EFI_EXIT(r);
 }
 
 static efi_status_t EFIAPI efi_protocols_per_handle(void *handle,
--
2.11.0

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v3 1/3] efi_loader: write protocol GUID in OpenProtocol

Alexander Graf
In reply to this post by Heinrich Schuchardt


On 05.08.17 21:32, Heinrich Schuchardt wrote:
> To understand what happens in OpenProtocol it is necessary to know
> the protocol interface GUID. Let's write a debug message.
>
> Using uuid_guid_get_str would be quite clumsy for this purpose.
> This would involve evaluating _DEBUG which probably should not be used
> outside common.h.
>
> Cc: Rob Clark <[hidden email]>
> Signed-off-by: Heinrich Schuchardt <[hidden email]>

I agree with Rob that a printf extension would be the nicest way to go
here. We could then just use that instead of the %p in EFI_ENTRY() that
we have today.


Alex
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v3 1/3] efi_loader: write protocol GUID in OpenProtocol

Heinrich Schuchardt
On 08/11/2017 12:11 PM, Alexander Graf wrote:

>
>
> On 05.08.17 21:32, Heinrich Schuchardt wrote:
>> To understand what happens in OpenProtocol it is necessary to know
>> the protocol interface GUID. Let's write a debug message.
>>
>> Using uuid_guid_get_str would be quite clumsy for this purpose.
>> This would involve evaluating _DEBUG which probably should not be used
>> outside common.h.
>>
>> Cc: Rob Clark <[hidden email]>
>> Signed-off-by: Heinrich Schuchardt <[hidden email]>
>
> I agree with Rob that a printf extension would be the nicest way to go
> here. We could then just use that instead of the %p in EFI_ENTRY() that
> we have today.
>
>
> Alex
>

Hello Alex,

I am aware of
[PATCH 4/5] vsprintf.c: add GUID printing,

I just wonder if you wnat to take this patch series as is and we modify
EFI_PRINT_GUID afterwards

or

we I shall remove GUID printing and resend the patch series without it.

Best regards

Heinrich
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v3 1/3] efi_loader: write protocol GUID in OpenProtocol

Alexander Graf


On 11.08.17 18:00, Heinrich Schuchardt wrote:

> On 08/11/2017 12:11 PM, Alexander Graf wrote:
>>
>>
>> On 05.08.17 21:32, Heinrich Schuchardt wrote:
>>> To understand what happens in OpenProtocol it is necessary to know
>>> the protocol interface GUID. Let's write a debug message.
>>>
>>> Using uuid_guid_get_str would be quite clumsy for this purpose.
>>> This would involve evaluating _DEBUG which probably should not be used
>>> outside common.h.
>>>
>>> Cc: Rob Clark <[hidden email]>
>>> Signed-off-by: Heinrich Schuchardt <[hidden email]>
>>
>> I agree with Rob that a printf extension would be the nicest way to go
>> here. We could then just use that instead of the %p in EFI_ENTRY() that
>> we have today.
>>
>>
>> Alex
>>
>
> Hello Alex,
>
> I am aware of
> [PATCH 4/5] vsprintf.c: add GUID printing,
>
> I just wonder if you wnat to take this patch series as is and we modify
> EFI_PRINT_GUID afterwards
>
> or
>
> we I shall remove GUID printing and resend the patch series without it.

I think it's best to keep changes isolated to not stall anyone. Since
this is really just a debug enhancement, I think we're best off to drop
the GUID printing in this patch set.


Alex
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v3 2/3] efi_loader: open_info in OpenProtocol, CloseProtocol

Alexander Graf
In reply to this post by Heinrich Schuchardt


On 05.08.17 22:32, Heinrich Schuchardt 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.
>
> Cc: Rob Clark <[hidden email]>
> Signed-off-by: Heinrich Schuchardt <[hidden email]>
> ---
> v3:
> use EFI_CALL to avoid wrapper for efi_disconnect_controller
> use list_for_each_entry
> move variable declarations out of loops
> v2:
> new patch
> ---
>   include/efi_loader.h          |   1 +
>   lib/efi_loader/efi_boottime.c | 164 +++++++++++++++++++++++++++++++++++++++---
>   2 files changed, 155 insertions(+), 10 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 553c615f11..222b251a38 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -88,6 +88,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 ebb557abb2..e969814476 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -898,23 +898,78 @@ static efi_status_t EFIAPI efi_connect_controller(
>   return EFI_EXIT(EFI_NOT_FOUND);
>   }
>  
> -static efi_status_t EFIAPI efi_disconnect_controller(void *controller_handle,
> -     void *driver_image_handle,
> -     void *child_handle)
> +static efi_status_t EFIAPI efi_disconnect_controller(
> + void *controller_handle,
> + void *driver_image_handle,
> + void *child_handle)
>   {
>   EFI_ENTRY("%p, %p, %p", controller_handle, driver_image_handle,
>    child_handle);
>   return EFI_EXIT(EFI_INVALID_PARAMETER);
>   }
>  
> +static efi_status_t efi_close_protocol_int(struct efi_handler *protocol,

Please either wrap _ext or use EFI_CALL :).

> +   void *agent_handle,
> +   void *controller_handle)
> +{
> + size_t i;
> + struct efi_open_protocol_info_entry *open_info;
> +
> + 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 &&
> +    open_info->controller_handle ==
> +    controller_handle) {
> + open_info->open_count--;
> + return EFI_SUCCESS;
> + }
> + }
> + return EFI_NOT_FOUND;
> +}
> +
>   static efi_status_t EFIAPI efi_close_protocol(void *handle,
>        efi_guid_t *protocol,
>        void *agent_handle,
>        void *controller_handle)
>   {
> + struct efi_object *efiobj;
> + size_t i;
> + efi_status_t r = EFI_NOT_FOUND;
> +
>   EFI_ENTRY("%p, %p, %p, %p", handle, protocol, agent_handle,
>    controller_handle);
> - return EFI_EXIT(EFI_NOT_FOUND);
> +
> + if (!handle || !protocol || !agent_handle) {
> + r = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> +
> + EFI_PRINT_GUID("protocol:", protocol);
> +
> + list_for_each_entry(efiobj, &efi_obj_list, 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)) {
> + r = efi_close_protocol_int(handler,
> +   agent_handle,
> +   controller_handle);
> + goto out;
> + }
> + }
> + goto out;
> + }
> +out:
> + return EFI_EXIT(r);
>   }
>  
>   static efi_status_t EFIAPI efi_open_protocol_information(efi_handle_t handle,
> @@ -1119,6 +1174,96 @@ static void EFIAPI efi_set_mem(void *buffer, unsigned long size, uint8_t value)
>   memset(buffer, value, size);
>   }
>  
> +static efi_status_t efi_open_protocol_int(

Same here.


Alex

> + 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,
> @@ -1173,12 +1318,11 @@ static efi_status_t EFIAPI efi_open_protocol(
>   if (!hprotocol)
>   continue;
>   if (!guidcmp(hprotocol, protocol)) {
> - if (attributes !=
> -    EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
> - *protocol_interface =
> - handler->protocol_interface;
> - }
> - r = EFI_SUCCESS;
> + r = efi_open_protocol_int(handler,
> +  protocol_interface,
> +  agent_handle,
> +  controller_handle,
> +  attributes);
>   goto out;
>   }
>   }
>
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v3 3/3] efi_loader: implement OpenProtocolInformation

Alexander Graf
In reply to this post by Heinrich Schuchardt


On 05.08.17 22:32, Heinrich Schuchardt 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 <[hidden email]>
> Signed-off-by: Heinrich Schuchardt <[hidden email]>

Do you have an application that leverages these interfaces? Would it be
possible to add that application to our travis tests?


Alex
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v3 2/3] efi_loader: open_info in OpenProtocol, CloseProtocol

Heinrich Schuchardt
In reply to this post by Alexander Graf
On 08/12/2017 03:37 PM, Alexander Graf wrote:

>
>
> On 05.08.17 22:32, Heinrich Schuchardt 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.
>>
>> Cc: Rob Clark <[hidden email]>
>> Signed-off-by: Heinrich Schuchardt <[hidden email]>
>> ---
>> v3:
>>     use EFI_CALL to avoid wrapper for efi_disconnect_controller
>>     use list_for_each_entry
>>     move variable declarations out of loops
>> v2:
>>     new patch
>> ---
>>   include/efi_loader.h          |   1 +
>>   lib/efi_loader/efi_boottime.c | 164
>> +++++++++++++++++++++++++++++++++++++++---
>>   2 files changed, 155 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 553c615f11..222b251a38 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -88,6 +88,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 ebb557abb2..e969814476 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -898,23 +898,78 @@ static efi_status_t EFIAPI efi_connect_controller(
>>       return EFI_EXIT(EFI_NOT_FOUND);
>>   }
>>   -static efi_status_t EFIAPI efi_disconnect_controller(void
>> *controller_handle,
>> -                             void *driver_image_handle,
>> -                             void *child_handle)
>> +static efi_status_t EFIAPI efi_disconnect_controller(
>> +                        void *controller_handle,
>> +                        void *driver_image_handle,
>> +                        void *child_handle)
>>   {
>>       EFI_ENTRY("%p, %p, %p", controller_handle, driver_image_handle,
>>             child_handle);
>>       return EFI_EXIT(EFI_INVALID_PARAMETER);
>>   }
>>   +static efi_status_t efi_close_protocol_int(struct efi_handler
>> *protocol,
>
> Please either wrap _ext or use EFI_CALL :).

Why?

Function efi_close_protocol_int is only used to avoid lines over 80
characters in efi_disconnect_controller. It is not called from anywhere
else.

Should I add some comment in the code or in the commit message?

>
>> +                       void *agent_handle,
>> +                       void *controller_handle)
>> +{
>> +    size_t i;
>> +    struct efi_open_protocol_info_entry *open_info;
>> +
>> +    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 &&
>> +            open_info->controller_handle ==
>> +            controller_handle) {
>> +            open_info->open_count--;
>> +            return EFI_SUCCESS;
>> +        }
>> +    }
>> +    return EFI_NOT_FOUND;
>> +}
>> +
>>   static efi_status_t EFIAPI efi_close_protocol(void *handle,
>>                             efi_guid_t *protocol,
>>                             void *agent_handle,
>>                             void *controller_handle)
>>   {
>> +    struct efi_object *efiobj;
>> +    size_t i;
>> +    efi_status_t r = EFI_NOT_FOUND;
>> +
>>       EFI_ENTRY("%p, %p, %p, %p", handle, protocol, agent_handle,
>>             controller_handle);
>> -    return EFI_EXIT(EFI_NOT_FOUND);
>> +
>> +    if (!handle || !protocol || !agent_handle) {
>> +        r = EFI_INVALID_PARAMETER;
>> +        goto out;
>> +    }
>> +
>> +    EFI_PRINT_GUID("protocol:", protocol);
>> +
>> +    list_for_each_entry(efiobj, &efi_obj_list, 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)) {
>> +                r = efi_close_protocol_int(handler,
>> +                               agent_handle,
>> +                               controller_handle);
>> +                goto out;
>> +            }
>> +        }
>> +        goto out;
>> +    }
>> +out:
>> +    return EFI_EXIT(r);
>>   }
>>     static efi_status_t EFIAPI
>> efi_open_protocol_information(efi_handle_t handle,
>> @@ -1119,6 +1174,96 @@ static void EFIAPI efi_set_mem(void *buffer,
>> unsigned long size, uint8_t value)
>>       memset(buffer, value, size);
>>   }
>>   +static efi_status_t efi_open_protocol_int(
>
> Same here.
>
>
> Alex

See above.

Was the rest of the patch ok for you?

Regards

Heinrich

>
>> +            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,
>> @@ -1173,12 +1318,11 @@ static efi_status_t EFIAPI efi_open_protocol(
>>               if (!hprotocol)
>>                   continue;
>>               if (!guidcmp(hprotocol, protocol)) {
>> -                if (attributes !=
>> -                    EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
>> -                    *protocol_interface =
>> -                        handler->protocol_interface;
>> -                }
>> -                r = EFI_SUCCESS;
>> +                r = efi_open_protocol_int(handler,
>> +                              protocol_interface,
>> +                              agent_handle,
>> +                              controller_handle,
>> +                              attributes);
>>                   goto out;
>>               }
>>           }
>>
>

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v3 3/3] efi_loader: implement OpenProtocolInformation

Heinrich Schuchardt
In reply to this post by Alexander Graf
On 08/12/2017 03:38 PM, Alexander Graf wrote:

>
>
> On 05.08.17 22:32, Heinrich Schuchardt 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 <[hidden email]>
>> Signed-off-by: Heinrich Schuchardt <[hidden email]>
>
> Do you have an application that leverages these interfaces? Would it be
> possible to add that application to our travis tests?
>
>
> Alex
>

iPXE (snp.efi) uses the functions but there are other things missing to
make it really working.

Putting new tests into the executable created by
CMD_BOOTEFI_HELLO_COMPILE would be possible but that seems messy to me.

Should we replace CMD_BOOTEFI_HELLO_COMPILE by an option that creates
multiple executables for the different EFI API functions we have to test?

Each test then should consist of:
- start QEMU system
- download dtb and test executable from tftp server
- start the test executable
- evaluate console output
- shutdown QEMU system

Regards

Heinrich
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v3 2/3] efi_loader: open_info in OpenProtocol, CloseProtocol

Alexander Graf
In reply to this post by Heinrich Schuchardt


On 13.08.17 13:09, Heinrich Schuchardt wrote:

> On 08/12/2017 03:37 PM, Alexander Graf wrote:
>>
>>
>> On 05.08.17 22:32, Heinrich Schuchardt 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.
>>>
>>> Cc: Rob Clark <[hidden email]>
>>> Signed-off-by: Heinrich Schuchardt <[hidden email]>
>>> ---
>>> v3:
>>>      use EFI_CALL to avoid wrapper for efi_disconnect_controller
>>>      use list_for_each_entry
>>>      move variable declarations out of loops
>>> v2:
>>>      new patch
>>> ---
>>>    include/efi_loader.h          |   1 +
>>>    lib/efi_loader/efi_boottime.c | 164
>>> +++++++++++++++++++++++++++++++++++++++---
>>>    2 files changed, 155 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>> index 553c615f11..222b251a38 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -88,6 +88,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 ebb557abb2..e969814476 100644
>>> --- a/lib/efi_loader/efi_boottime.c
>>> +++ b/lib/efi_loader/efi_boottime.c
>>> @@ -898,23 +898,78 @@ static efi_status_t EFIAPI efi_connect_controller(
>>>        return EFI_EXIT(EFI_NOT_FOUND);
>>>    }
>>>    -static efi_status_t EFIAPI efi_disconnect_controller(void
>>> *controller_handle,
>>> -                             void *driver_image_handle,
>>> -                             void *child_handle)
>>> +static efi_status_t EFIAPI efi_disconnect_controller(
>>> +                        void *controller_handle,
>>> +                        void *driver_image_handle,
>>> +                        void *child_handle)
>>>    {
>>>        EFI_ENTRY("%p, %p, %p", controller_handle, driver_image_handle,
>>>              child_handle);
>>>        return EFI_EXIT(EFI_INVALID_PARAMETER);
>>>    }
>>>    +static efi_status_t efi_close_protocol_int(struct efi_handler
>>> *protocol,
>>
>> Please either wrap _ext or use EFI_CALL :).
>
> Why?
>
> Function efi_close_protocol_int is only used to avoid lines over 80
> characters in efi_disconnect_controller. It is not called from anywhere
> else.
>
> Should I add some comment in the code or in the commit message?

Ah, now I see. No, I think the function name is misleading, so that
needs change :). How about efi_close_one_protocol()?

>
>>
>>> +                       void *agent_handle,
>>> +                       void *controller_handle)
>>> +{
>>> +    size_t i;
>>> +    struct efi_open_protocol_info_entry *open_info;
>>> +
>>> +    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 &&
>>> +            open_info->controller_handle ==
>>> +            controller_handle) {
>>> +            open_info->open_count--;
>>> +            return EFI_SUCCESS;
>>> +        }
>>> +    }
>>> +    return EFI_NOT_FOUND;
>>> +}
>>> +
>>>    static efi_status_t EFIAPI efi_close_protocol(void *handle,
>>>                              efi_guid_t *protocol,
>>>                              void *agent_handle,
>>>                              void *controller_handle)
>>>    {
>>> +    struct efi_object *efiobj;
>>> +    size_t i;
>>> +    efi_status_t r = EFI_NOT_FOUND;
>>> +
>>>        EFI_ENTRY("%p, %p, %p, %p", handle, protocol, agent_handle,
>>>              controller_handle);
>>> -    return EFI_EXIT(EFI_NOT_FOUND);
>>> +
>>> +    if (!handle || !protocol || !agent_handle) {
>>> +        r = EFI_INVALID_PARAMETER;
>>> +        goto out;
>>> +    }
>>> +
>>> +    EFI_PRINT_GUID("protocol:", protocol);
>>> +
>>> +    list_for_each_entry(efiobj, &efi_obj_list, 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)) {
>>> +                r = efi_close_protocol_int(handler,
>>> +                               agent_handle,
>>> +                               controller_handle);
>>> +                goto out;
>>> +            }
>>> +        }
>>> +        goto out;
>>> +    }
>>> +out:
>>> +    return EFI_EXIT(r);
>>>    }
>>>      static efi_status_t EFIAPI
>>> efi_open_protocol_information(efi_handle_t handle,
>>> @@ -1119,6 +1174,96 @@ static void EFIAPI efi_set_mem(void *buffer,
>>> unsigned long size, uint8_t value)
>>>        memset(buffer, value, size);
>>>    }
>>>    +static efi_status_t efi_open_protocol_int(
>>
>> Same here.
>>
>>
>> Alex
>
> See above.
>
> Was the rest of the patch ok for you?

I didn't spot anything obviously bad, but that doesn't usually mean
much. My review foo isn't quite as good as others'. When applying I
would push the patches through some more detailed testing though.


Alex
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v3 3/3] efi_loader: implement OpenProtocolInformation

Alexander Graf
In reply to this post by Heinrich Schuchardt


On 13.08.17 13:17, Heinrich Schuchardt wrote:

> On 08/12/2017 03:38 PM, Alexander Graf wrote:
>>
>>
>> On 05.08.17 22:32, Heinrich Schuchardt 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 <[hidden email]>
>>> Signed-off-by: Heinrich Schuchardt <[hidden email]>
>>
>> Do you have an application that leverages these interfaces? Would it be
>> possible to add that application to our travis tests?
>>
>>
>> Alex
>>
>
> iPXE (snp.efi) uses the functions but there are other things missing to
> make it really working.

Ah, I see. How much is missing to make that one work for real?

> Putting new tests into the executable created by
> CMD_BOOTEFI_HELLO_COMPILE would be possible but that seems messy to me.
>
> Should we replace CMD_BOOTEFI_HELLO_COMPILE by an option that creates
> multiple executables for the different EFI API functions we have to test?
>
> Each test then should consist of:
> - start QEMU system
> - download dtb and test executable from tftp server
> - start the test executable
> - evaluate console output
> - shutdown QEMU system

We have most of the above already in travis today. All we would need is
to extend it to download a built iPXE binaries and add test snippets to
tests/py for iPXE functionality.


Alex
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v3 3/3] efi_loader: implement OpenProtocolInformation

Heinrich Schuchardt-2
On 08/13/2017 09:24 PM, Alexander Graf wrote:

>
>
> On 13.08.17 13:17, Heinrich Schuchardt wrote:
>> On 08/12/2017 03:38 PM, Alexander Graf wrote:
>>>
>>>
>>> On 05.08.17 22:32, Heinrich Schuchardt 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 <[hidden email]>
>>>> Signed-off-by: Heinrich Schuchardt <[hidden email]>
>>>
>>> Do you have an application that leverages these interfaces? Would it be
>>> possible to add that application to our travis tests?
>>>
>>>
>>> Alex
>>>
>>
>> iPXE (snp.efi) uses the functions but there are other things missing to
>> make it really working.
>
> Ah, I see. How much is missing to make that one work for real?

Before reaching the input prompt ConnectController and
DisconnectController fail for the Simple Network Protocol.

So I am not able to say if the network connection will work.

For testing we will need an iSCSI target.

>
>> Putting new tests into the executable created by
>> CMD_BOOTEFI_HELLO_COMPILE would be possible but that seems messy to me.
>>
>> Should we replace CMD_BOOTEFI_HELLO_COMPILE by an option that creates
>> multiple executables for the different EFI API functions we have to test?
>>
>> Each test then should consist of:
>> - start QEMU system
>> - download dtb and test executable from tftp server
>> - start the test executable
>> - evaluate console output
>> - shutdown QEMU system
>
> We have most of the above already in travis today. All we would need is
> to extend it to download a built iPXE binaries and add test snippets to
> tests/py for iPXE functionality.

Proving that some binary runs is not enough to test the different corner
cases of this complex API.

I would prefer to have a bunch of test binaries compiled from the U-Boot
git tree.

Best regards

Heinrich
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v3 3/3] efi_loader: implement OpenProtocolInformation

Alexander Graf


On 13.08.17 21:32, Heinrich Schuchardt wrote:

> On 08/13/2017 09:24 PM, Alexander Graf wrote:
>>
>>
>> On 13.08.17 13:17, Heinrich Schuchardt wrote:
>>> On 08/12/2017 03:38 PM, Alexander Graf wrote:
>>>>
>>>>
>>>> On 05.08.17 22:32, Heinrich Schuchardt 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 <[hidden email]>
>>>>> Signed-off-by: Heinrich Schuchardt <[hidden email]>
>>>>
>>>> Do you have an application that leverages these interfaces? Would it be
>>>> possible to add that application to our travis tests?
>>>>
>>>>
>>>> Alex
>>>>
>>>
>>> iPXE (snp.efi) uses the functions but there are other things missing to
>>> make it really working.
>>
>> Ah, I see. How much is missing to make that one work for real?
>
> Before reaching the input prompt ConnectController and
> DisconnectController fail for the Simple Network Protocol.
>
> So I am not able to say if the network connection will work.
>
> For testing we will need an iSCSI target.
>
>>
>>> Putting new tests into the executable created by
>>> CMD_BOOTEFI_HELLO_COMPILE would be possible but that seems messy to me.
>>>
>>> Should we replace CMD_BOOTEFI_HELLO_COMPILE by an option that creates
>>> multiple executables for the different EFI API functions we have to test?
>>>
>>> Each test then should consist of:
>>> - start QEMU system
>>> - download dtb and test executable from tftp server
>>> - start the test executable
>>> - evaluate console output
>>> - shutdown QEMU system
>>
>> We have most of the above already in travis today. All we would need is
>> to extend it to download a built iPXE binaries and add test snippets to
>> tests/py for iPXE functionality.
>
> Proving that some binary runs is not enough to test the different corner
> cases of this complex API.
>
> I would prefer to have a bunch of test binaries compiled from the U-Boot
> git tree.

Sure, even better in my book ;). Ideally we would have both. Unit tests
to check individual interfaces and integration tests to make sure we
don't regress real world use cases.


Alex
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Loading...